The code so bad you won't hire

[ParadigmShifter]

It's returning the earliest transaction that hasn't been paid by the due date?

Looks like it's some sort of iterator/coroutine though(since it advances the iterator to the list). Probably better to cache the sorted list maybe than constantly resorting it?

Looks like Java anyway, ugh

[SteveSmith]

Since we are now discussing bloated code... here's another routine I had to fix in production:

Code: Select all

public OpenItem getPaymentNotMade(Collection payments) {
    final List paymentsAsList = new ArrayList(payments);
    Collections.sort(paymentsAsList, new Comparator() {
        public int compare(Object o1, Object o2) {
            try {
                final OpenItem openItem1 = (OpenItem)o1;
                final OpenItem openItem2 = (OpenItem)o2;
                if
(((OpenItem.STATUS_ACTIVE.equals(openItem1.getStatus())) &&
(OpenItem.SUBSTATUS_ACTIVE.equals(openItem1.getSubStatus())))
                        &&
((OpenItem.STATUS_ACTIVE.equals(openItem2.getStatus())) &&
(OpenItem.SUBSTATUS_ACTIVE.equals(openItem2.getSubStatus())))) {
                    if (openItem1.getDueDate().before(openItem2.getDueDate())) {
                        return (1);
                    }
                }
                return (-1);
            } catch (Exception ex) {
                throw (new RuntimeException(ex));
            }
        }
    });
    return ((OpenItem)paymentsAsList.iterator().next());
}

Bonus points to the first person that can explain what's going on!

Seems to return the earliest non-ACTIVE OpenItem*

(* This may not be entirely correct, I only had a quick look)

[Dr beep]

Ohhh come on...
you just post the code here so we can do your job for this week.

Since we are now discussing bloated code... here's another routine I had to fix in production:

Code: Select all

public OpenItem getPaymentNotMade(Collection payments) {
    final List paymentsAsList = new ArrayList(payments);
    Collections.sort(paymentsAsList, new Comparator() {
        public int compare(Object o1, Object o2) {
            try {
                final OpenItem openItem1 = (OpenItem)o1;
                final OpenItem openItem2 = (OpenItem)o2;
                if
(((OpenItem.STATUS_ACTIVE.equals(openItem1.getStatus())) &&
(OpenItem.SUBSTATUS_ACTIVE.equals(openItem1.getSubStatus())))
                        &&
((OpenItem.STATUS_ACTIVE.equals(openItem2.getStatus())) &&
(OpenItem.SUBSTATUS_ACTIVE.equals(openItem2.getSubStatus())))) {
                    if (openItem1.getDueDate().before(openItem2.getDueDate())) {
                        return (1);
                    }
                }
                return (-1);
            } catch (Exception ex) {
                throw (new RuntimeException(ex));
            }
        }
    });
    return ((OpenItem)paymentsAsList.iterator().next());
}

Bonus points to the first person that can explain what's going on!

.

[ParadigmShifter]

Anyway it only seems to be sorting elements that are active dunno what would happen if there are inactive ones in the collection (looks like it puts them at the beginning of the list since it's returning -1 as the comparison result?).

So it should probably only add active items to the list before sorting I expect?

[Oloturia]

That's probably how I'd have done it in Sinclair BASIC!

I'm not a proper coder - whats so wrong with that example? It works for all eventualities doesn't it?

This reminds me of a story I read about a student whose assignment was to write a code for a calculator that had to do basic operations.

He complained it was a huge job and he wasn't even close to coding half the operations.
He did not mean that he had done addition and subtraction. He was literally coding every operation as:
if a=1 and b=2 then solution=3
if a=1 and b=3 then solution=4
etc.

[bluespikey]

I think it will return the earliest document amongst all the active documents.

Not used Java for a long while. It would be easier to filter the list to find active documents, then sort that list based on the due date. Should only be two lines of code.

[Einar Saukas]

It's returning the earliest transaction that hasn't been paid by the due date?

I agree it makes more sense for this routine to return the earliest transaction (that will expire first). But this implementation is doing the opposite.

If the list contains only ACTIVE transactions, then it will return the latest transaction.

Anyway it only seems to be sorting elements that are active dunno what would happen if there are inactive ones in the collection (looks like it puts them at the beginning of the list since it's returning -1 as the comparison result?).

Good catch!

If the list contains at least one non-ACTIVE transaction, then this routine is broken. It may return anything.

So it should probably only add active items to the list before sorting I expect?

Nope.

If this routine was supposed to be used in a list that contains ACTIVE transactions only, then why is it checking ACTIVE at every transaction comparison?

From other parts of the system, I know this routine is supposed to find the ACTIVE transaction that will expire first, from a list of ACTIVE and non-ACTIVE transactions. It never worked as expected

Looks like it's some sort of iterator/coroutine though(since it advances the iterator to the list). Probably better to cache the sorted list maybe than constantly resorting it?

Actually there's no need to sort anything! It's simpler and much faster to simply traverse the list once, like this:

Code: Select all

public OpenItem getEarliestPaymentNotMade(Collection payments) {
    OpenItem earliest = null;
    for (OpemItem payment : payments) {
        if (payment.getStatus() == OpenItem.STATUS_ACTIVE && payment.getSubStatus() == OpenItem.SUBSTATUS_ACTIVE) {
            if (earliest == null || payment.getDueDate().before(earliest.getDueDate())) {
                earliest = payment;
            }
        }
    }
    return earliest;
}

Or you could always keep all payments in a permanently sorted list (sorted by date but with ACTIVE payments first). Although this option wasn't viable in this system.

[SteveSmith]

Ideally, shouldn't this be offloaded to a db, and just run an SQL Select on it?

[Einar Saukas]

It would be easier to filter the list to find active documents, then sort that list based on the due date. Should only be two lines of code.

Good point! Although even with streams, there's no need for sorting:

Code: Select all

return payments.stream()
    .filter(p -> p.getStatus() == OpenItem.STATUS_ACTIVE && p.getSubStatus() == OpenItem.SUBSTATUS_ACTIVE)
    .min(Comparator.comparing(OpenItem::getDueDate)).orElse(null);

The code above has the advantage of being easier and shorter. However it's less efficient since it requires traversing the list twice.

Anyway I worked on this system over a decade ago, when Java streams didn't exist yet.

[R-Tape]

This reminds me of a story I read about a student whose assignment was to write a code for a calculator that had to do basic operations.

He complained it was a huge job and he wasn't even close to coding half the operations.
He did not mean that he had done addition and subtraction. He was literally coding every operation as:
if a=1 and b=2 then solution=3
if a=1 and b=3 then solution=4
etc.

Add a million or so GOTOs and we have the SQIJ approach!

[Einar Saukas]

Ideally, shouldn't this be offloaded to a db, and just run an SQL Select on it?

In case of very large distributed systems, you want to do the opposite: keep database operations to a minimum (only for operations that require database integrity/consistency) and process everything else locally on application servers.

The reason is, if the workload increases (for instance imagine what happens on a Black Friday), you can quickly and easily scale up your system by allocating more application servers. But changing the capacity of a database is a nightmare, because there are limits on how much you can increase it, usually you cannot reduce it again afterwards, it's a lot more expensive, etc...

[Dr beep]

I once had to test a system as a user.

In a database:
You could do a selection and a sort.
The programmer first did the sort and then he selected the items he needed.

I suggested to do it the other way around, thus only sorting the selected records.
He was not amused and asked if I could be taken of the testgroup.

[Einar Saukas]

Since we are now discussing sorting... Another routine they had in production:

Code: Select all

    /**
     * Sorts the list formed by specified <code>Set</code> according to the order induced by the > comparator.
     *
     * @param c The <code>Set</code> to be sorted.
     * @return The ordered Collection
     */
    private static Collection getSortList(Set c) {
        List lista = null;
        if (c != null) {
            lista = Arrays.asList(c.toArray());
            int x = 0;
            for (int i = 0; i < lista.size(); i++) {
                Process p1 = (Process)lista.get(x);
                Process p = (Process)lista.get(i);
                if (p.getProcessId() > p1.getProcessId()) {
                    // Swap
                    lista.set(x, p);
                    lista.set(i, p1);
                }
                if (i == lista.size() - 1) {
                    x++;
                    i = 0;
                }
                if (x == lista.size() - 1) {
                    break;
                }
            }
        }
        return lista;
    }

More bonus points to whoever figures out what's going on!

[ParadigmShifter]

Looks like it's doing a bubble sort (O(n^2), same complexity as quicksort worst case) on process id whatever that is.

Dunno if Set has a sort method or if a set is already sorted in Java.

Comparing each element to itself as well.

Should probably use a better container and just populate it from the Set. In C++ I would put it in a std::map since each element of a set is unique and a std::map iterator returns results in ascending order. Dunno if Java has something similar (a red/black tree) since if it only has hash maps they are not sorted by key.

I think sets in C++ are usually red/black trees as well so would be sorted by default... might be implementer specific I dunno. EDIT: Looks like they are stored in ascending key order so in C++ std::sets are already sorted.

I see it is also making an Array out of the set members and then making a List out of that lol. Why not just sort the array? Or the List? In C++ std::list has it's own sort function since std::sort requires random access iterators which a List does not have. Dunno what algorithm it likely uses to do the sort though, certainly not bubble sort anyway, since the standard guarantees order O(N log N)

Dunno if a List in Java is really a resizable array though (so not actually a linked list, same as in C#).

I do know Java containers suck though with all the boxing/unboxing they have to do (unless they have fixed generics recently). At least C# has proper generics which don't require boxing.

EDIT: I see Java has a SortedSet container should probably have used that.

EDIT: Oh and it's accessing the List by index so I really hope a List isn't actually a linked list

Another EDIT: We had a guy who was sorting a list of moving objects (cars in the background) every frame in terms of distance travelled along the path (for cars in the background driving up roads on fixed paths).

1) The cars could not change track or overtake so if the positions along the path changed with respect to others you'd be in serious trouble anyway (they would pass through each other, they were not doing collision or anything). Just use a ring buffer and recycle when cars reached the end of the path
2) Sorting an already sorted list causes worst case behaviour if it uses quicksort, so lets hope it didn't use that.

[Einar Saukas]

Looks like it's doing a bubble sort (O(n^2), same complexity as quicksort worst case) on process id whatever that is.

Sure, that's what it's trying to do. But can you tell what it actually does???

[Einar Saukas]

About your other questions... There are several implementations of Set in Java (for instance the sorted red-black tree implementation is called TreeSet), but you can ignore all Java specifics and just consider the problem of converting an unordered set into a sorted array.

I'm not trying to test anyone's knowledge of Java. My intent was showing examples of terrible code regardless of language.

I see it is also making an Array out of the set members and then making a List out of that lol. Why not just sort the array? Or the List?

Sure. But this could be explained as lack of familiarity with Java, so I don't consider it terrible code. The problem is elsewhere.

Oh and it's accessing the List by index so I really hope a List isn't actually a linked list

There are also several implementations of List in Java. In this case it's an array, not a linked list.

[equinox]

When I saw that awful post-office Visual Basic (lol) that took 10 lines to do unary minus, my first thought was: AHA, clearly this is a KLOC situation (i.e. being paid per line of code, so encouraged to write shitty algorithms that take many lines). I'm not sure, it might conceivably have just been a very clueless person, but, errr, Occam's razor?

[equinox]

I once had to test a system as a user.

In a database:
You could do a selection and a sort.
The programmer first did the sort and then he selected the items he needed.

I suggested to do it the other way around, thus only sorting the selected records.
He was not amused and asked if I could be taken of the testgroup.

Users and programmers must never be simultaneously in the same building.
There's "async" now to get their arses outside. Their parallelly sorted arses.

[Einar Saukas]

Sure, that's what it's trying to do. But can you tell what it actually does???

The answer is, this routine will generate a list of elements in pseudo-random order, except for the first element that will have the highest process id.

Thus it's not sorting anything! Certainly not the intended behavior:)


PS: My apologies for the delay, I'm on vacation right now and forgot to follow up on this...

[ParadigmShifter]

It seems to compare everything to everything else so where exactly is the bug?

Partial sort when done properly is a perfectly valid algorithm though (say you wanted the top 10 items and didn't care about the rest, a priority queue where you only really care about the highest priority value each iteration, etc.)

https://en.cppreference.com/w/cpp/algor ... rtial_sort

[sn3j]

It's the first pass of bubble sort over and over. It never gets the maximum of all process ids out of hand, so the rest remains unsorted.

[Einar Saukas]

It seems to compare everything to everything else so where exactly is the bug?

This routine compares elements at positions i and x, moving the highest processId to position x. The problem is, sometimes x < i and sometimes x > i. Therefore elements are moved back and forth without following any logic.

My theory is, the developers considered themselves smarter than everybody else. They saw the Bubble sort implementation using 2 nested loops and thought they could do better using a single loop. But they didn't know what they were doing. Their implementation is at least 2 times slower and doesn't sort anything.

Their arrogance would also explain why they decided to implement their own sort algorithm, instead of simply using the (much faster) native sort in Java:

Code: Select all

List<Process> list = new ArrayList<>(c);      // convert to list
list.sort(comparing(Process::getProcessId));  // sort the list

(with small changes if you want descending order instead)

[Einar Saukas]

Partial sort when done properly is a perfectly valid algorithm though

That's not the case here. A "top 1" algorithm in O(n^2) makes no sense.

In this case, the first element only works due to a bug. I'm sure they didn't notice that variable i would be incremented immediately after it was set back to zero. This prevents the first element from being moved back and forth, like the others.

[ParadigmShifter]

Well moral of that story is never ship your own implementation of a standard algorithm without first

a) verify that the new algorithm does the same thing as the old one

and

b) verify that is actually more efficient if it does work

Some sorting algorithms can perform better than how sort is usually implemented (either quicksort or mergesort variants) so it is sometimes worth writing your own implementation in certain cases.

EDIT: Re: previous post yeah, that's why I said "when done properly" i.e. not O(n^2) C++ standard library thought partial_sort was worthy of inclusion anyway.

[catmeows]

Well moral of that story is never ship your own implementation of a standard algorithm without first

And write unit tests. I have seen so many bugs that would be discovered even by the simplest test.