The code so bad you won't hire
- ParadigmShifter
- Manic Miner
- Posts: 671
- Joined: Sat Sep 09, 2023 4:55 am
Re: The code so bad you won't hire
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
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
-
- Manic Miner
- Posts: 724
- Joined: Mon Nov 26, 2018 1:07 pm
- Location: UK
- Contact:
Re: The code so bad you won't hire
Seems to return the earliest non-ACTIVE OpenItem*Einar Saukas wrote: ↑Thu Jan 18, 2024 1:15 pm Since we are now discussing bloated code... here's another routine I had to fix in production:
Bonus points to the first person that can explain what's going on!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()); }
(* This may not be entirely correct, I only had a quick look)
Multiplayer Feud: https://stephensmith.itch.io/feud-online
Re: The code so bad you won't hire
Ohhh come on...
you just post the code here so we can do your job for this week.
you just post the code here so we can do your job for this week.
.Einar Saukas wrote: ↑Thu Jan 18, 2024 1:15 pm Since we are now discussing bloated code... here's another routine I had to fix in production:
Bonus points to the first person that can explain what's going on!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()); }
- ParadigmShifter
- Manic Miner
- Posts: 671
- Joined: Sat Sep 09, 2023 4:55 am
Re: The code so bad you won't hire
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?
So it should probably only add active items to the list before sorting I expect?
Re: The code so bad you won't hire
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
- Manic Miner
- Posts: 954
- Joined: Tue Jun 30, 2020 3:54 pm
Re: The code so bad you won't hire
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.
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
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
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.ParadigmShifter wrote: ↑Thu Jan 18, 2024 1:23 pm It's returning the earliest transaction that hasn't been paid by the due date?
If the list contains only ACTIVE transactions, then it will return the latest transaction.
Good catch!ParadigmShifter wrote: ↑Thu Jan 18, 2024 1:30 pm 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?).
If the list contains at least one non-ACTIVE transaction, then this routine is broken. It may return anything.
Nope.ParadigmShifter wrote: ↑Thu Jan 18, 2024 1:30 pm So it should probably only add active items to the list before sorting I expect?
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
Actually there's no need to sort anything! It's simpler and much faster to simply traverse the list once, like this:ParadigmShifter wrote: ↑Thu Jan 18, 2024 1:30 pm 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?
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;
}
-
- Manic Miner
- Posts: 724
- Joined: Mon Nov 26, 2018 1:07 pm
- Location: UK
- Contact:
Re: The code so bad you won't hire
Ideally, shouldn't this be offloaded to a db, and just run an SQL Select on it?
Multiplayer Feud: https://stephensmith.itch.io/feud-online
- Einar Saukas
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
Good point! Although even with streams, there's no need for sorting:bluespikey wrote: ↑Thu Jan 18, 2024 2:31 pm 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.
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);
Anyway I worked on this system over a decade ago, when Java streams didn't exist yet.
Re: The code so bad you won't hire
Add a million or so GOTOs and we have the SQIJ approach!Oloturia wrote: ↑Thu Jan 18, 2024 2:25 pm 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.
- Einar Saukas
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
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.SteveSmith wrote: ↑Thu Jan 18, 2024 3:18 pm Ideally, shouldn't this be offloaded to a db, and just run an SQL Select on it?
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...
Re: The code so bad you won't hire
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.
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
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
Since we are now discussing sorting... Another routine they had in production:
More bonus points to whoever figures out what's going on!
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;
}
- ParadigmShifter
- Manic Miner
- Posts: 671
- Joined: Sat Sep 09, 2023 4:55 am
Re: The code so bad you won't hire
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.
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
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
Sure, that's what it's trying to do. But can you tell what it actually does???ParadigmShifter wrote: ↑Thu Jan 18, 2024 6:36 pm Looks like it's doing a bubble sort (O(n^2), same complexity as quicksort worst case) on process id whatever that is.
- Einar Saukas
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
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'm not trying to test anyone's knowledge of Java. My intent was showing examples of terrible code regardless of language.
Sure. But this could be explained as lack of familiarity with Java, so I don't consider it terrible code. The problem is elsewhere.ParadigmShifter wrote: ↑Thu Jan 18, 2024 6:36 pm 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?
There are also several implementations of List in Java. In this case it's an array, not a linked list.ParadigmShifter wrote: ↑Thu Jan 18, 2024 6:36 pm Oh and it's accessing the List by index so I really hope a List isn't actually a linked list
Re: The code so bad you won't hire
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?
Re: The code so bad you won't hire
Users and programmers must never be simultaneously in the same building.Dr beep wrote: ↑Thu Jan 18, 2024 6:00 pm 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.
There's "async" now to get their arses outside. Their parallelly sorted arses.
- Einar Saukas
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
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.Einar Saukas wrote: ↑Fri Jan 19, 2024 11:57 pm Sure, that's what it's trying to do. But can you tell what it actually does???
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
- Manic Miner
- Posts: 671
- Joined: Sat Sep 09, 2023 4:55 am
Re: The code so bad you won't hire
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
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
Re: The code so bad you won't hire
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.
POKE 23614,10: STOP 1..0 hold, SS/m/n colors, b/spc toggle
- Einar Saukas
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
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.ParadigmShifter wrote: ↑Sat Jan 27, 2024 11:39 am It seems to compare everything to everything else so where exactly is the bug?
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
- Einar Saukas
- Bugaboo
- Posts: 3146
- Joined: Wed Nov 15, 2017 2:48 pm
Re: The code so bad you won't hire
That's not the case here. A "top 1" algorithm in O(n^2) makes no sense.ParadigmShifter wrote: ↑Sat Jan 27, 2024 11:39 am Partial sort when done properly is a perfectly valid algorithm though
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
- Manic Miner
- Posts: 671
- Joined: Sat Sep 09, 2023 4:55 am
Re: The code so bad you won't hire
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.
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.
Re: The code so bad you won't hire
And write unit tests. I have seen so many bugs that would be discovered even by the simplest test.ParadigmShifter wrote: ↑Sat Jan 27, 2024 3:29 pm Well moral of that story is never ship your own implementation of a standard algorithm without first
Proud owner of Didaktik M