
Reviewing the worst piece of code ever - micheleriva
https://www.micheleriva.it/posts/2020-07-31-reviewing-the-worst-piece-of-code-ever
======
bane
Ha, this is nothing. We once fired a programmer and had her turn over her code
after she failed to turn any work in after a few months. Her code was
atrocious and also none of it worked...or even was runnable. The biggest sin
was that she wrote all of her code in MS-Word, smart-quotes and all on all the
literals. Never once ever tried to run or test any code, just thousands of
lines of useless stuff that "looked" like code.

Oh yeah, she's also an adjunct CS professor at a local college.

~~~
colmvp
> We once fired a programmer and had her turn over her code after she failed
> to turn any work in after a few months

How did that go un-noticed for months?

~~~
mercer
We had a guy who, after a month of sitting smack-dab in the center of the IT
department being busy on his new MacBook, got caught with his pants down
because he finally asked for help, and me and a bunch of other devs stood
behind him watched in horror as we discovered that not only did he know
nothing about programming, he didn't know /any/ keyboard shortcuts.

When we asked him to copy some code from one file to another, he'd click on
'edit' and 'copy', struggle to get to the other file, and then click on 'edit'
and 'paste'.

How it went unnoticed is that we were all busy with our own stuff, he was put
on some small project that none of us were working on, and we just assumed
that nobody would be stupid enough to hire someone without at least /some/
vetting.

He slipped through the cracks because he got hired into another department,
and when they found out that he'd worked as a 'developer' at Microsoft they
figured he'd be more useful in our department.

In hindsight I feel I should've known something wasn't quite right when every
time I walked past his brand-new MacBook, he seemed to /really/ be struggling
with something and it never looked like it was code. I imagine he spent the
entire month figuring out how to deal with not having a 'start' button.

EDIT: I'll add that while this was in my top 10 of worst situations, there are
many others that, for me, provided a convincing argument against the idea that
corporations are somehow more efficient or whatever than the government.

My experience with both the public and private sector, beyond a certain size
at least, is that they're largely similar in
wastefulness/inefficiency/idiocy/etc.

~~~
onionisafruit
That’s what I felt like when ai joined a company that issued me a macbook for
the first time. There’s nothing quite like demonstrating your code to somebody
and having the conversation turn into a tutorial on cmd vs ctrl keys. It was
probably only a week or two before I had it mostly worked out, but the
imposter syndrome was pretty bad for a while. I can imagine falling into the
same trap as the fellow in your story if I had trouble with the code too. At
some point you start thinking that anything you say will out you so you just
shut up and hope to delay the inevitable.

~~~
agustif
Been the same for me using a Windows PC on work having a Mac home.

having different keymappings in different machines is hard

~~~
RulerOf
I was okay when I had the proper keyboard in front of me, but I still stumble
with reflexively using the correct keys for shortcuts when I'm remote
controlling one from the other.

I've found it very difficult to use Ctrl+C to copy from Windows and then Cmd+V
to paste on Mac OS without pretty frequently using the wrong modifier key.

~~~
agustif
Also, Hyper.app has ctrl+shift+v/c for copy paste, Git-ui has some weird shit
like alt-shift-fn-c/v too LOL...

------
Izkata
Code formatting is indeed minor here compared to the rest, but you missed the
one that popped out at me: The opening curly braces are sometimes on the same
line as the condition, sometimes on the following line.

That said,

> I’m absolutely sure that the code above is fake.

> That’s the first time that I see a synchronous SQL query:
    
    
      var accounts = apiService.sql(
        "SELECT * FROM users"
      );
    

Pretty sure you meant synchronous ajax/web request rather than sql query. This
could be old code, because it totally used the be a thing:
[https://developer.mozilla.org/en-
US/docs/Web/API/XMLHttpRequ...](https://developer.mozilla.org/en-
US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests#Synchronous_request)

Edit: Actually hell, it still works, at least in Firefox. It just also logs
the deprecation warning mentioned on that page. I thought this was already
removed.

~~~
lolc
Funny I almost commented that you must be mistaken because it would be
positively insane to support synchronous requests. Wow, that is insane.

In the end, synchronization can be achieved by busy waiting. So the
implementation of apiService.sql() could just rely on that even if the browser
API didn't support it.

The code still looks fake to me. Because you have to know quite a bit to
create a combination of fails like that. Natural bad code I've seen is less
purposefully structured. But maybe I just haven't been exposed to this type.

~~~
Izkata
> In the end, synchronization can be achieved by busy waiting. So the
> implementation of apiService.sql() could just rely on that even if the
> browser API didn't support it.

I haven't tested this, but I don't think it can. The result callback doesn't
have a chance to run if there's a busy loop - javascript is single-threaded,
and callbacks only fire when idle (think cooperative multitasking, not
preemptive multitasking).

Webworkers may get around this, but I've not used them and am not sure of the
details on how they work. A quick look at a guide indicates they still rely on
event callbacks in the main thread, so if my thought above is right, it looks
like they wouldn't work either.

~~~
lolc
It's true! I tested it because I thought it would be possible to just wait for
readyState == 4 in a loop. My assumption was that XMLHttpRequest runs in
another thread once you do send() on it. And indeed, the request starts
immediately. But the instance of XMLHttpRequest doesn't get updated until
control is yielded. So you can't get the goods. All considered, that is a good
thing :-)

------
Sniffnoy
One bit of awfulness the article missed: Rather than scanning for the username
and then checking whether the password is correct, it scans for a match on
both username _and_ password. So if you have the right username but the wrong
password, it still has to check _every single_ username to conclude this; it
won't stop when it gets to the username you entered.

I mean, not that it should be scanning through a list to find the right
username, but still...

(And yeah, I suppose that would allow for some sort of username enumeration
via a timing attack, but somehow I don't think that's the reason they didn't
do that here...)

~~~
champtar
You assume that username are unique in this DB, maybe they support multiple
password :)

~~~
onemiketwelve
What a neat feature ;)

~~~
Viliam1234
If you forget a new password, you can create a new one, and then if you
remember the old password later, you can use both. Is this so different in
principle from supporting multiple methods of authentication? :D

------
jlengrand
A colleague of mine once turned in A FULL JAVA APPLICATION all written in the
static void main method. Using booleans in databases polled at 25k/s to check
for completion of async tasks. Worst is, it was working. I spent 6 months
rewriting it, using it as a black blox to recreate the same system, including
bugs because we were interfacing with other systems.

Infuriating given that we were a team people team, but also a great deal of
fun from the engineering's side.

EDIT: Forgot to mention I was the junior dev and he had in excess of 15 years
of experience :D

~~~
kevsim
A company I worked for acquired another company. When we finally got access to
their code it was in two files: source.cpp and source2.cpp. The latter was
introduced when they reached some limit of the visual c++ compiler at the
time.

~~~
jlengrand
Holy cow hahaha.

------
anamexis
Hilarious, and a great breakdown of what's wrong with the code!

But also, in an article calling out programming falsehoods, this snippet is a
bit odd:

> Even if apiService.sql returns a value synchronously (which I doubt),
> internally it have to open a connection to a database, make a query and send
> back the response, which (as you may have guessed) can’t be synchronous.

Sure it can, it's dead simple to write an API that accepts a database query
and returns the result synchronously.

~~~
ErikAugust
But how would you call this API from the browser synchronously? I guess in
theory there is a synchronous XHR call but I’ve never seen it used. That could
be something you add to the problems.

~~~
Izkata
In the early days of ajax proliferation, I remember reading blog posts where
people recommended synchronous XHR to keep your code linear, instead of trying
to understand callbacks, which would make the code too complicated.

~~~
bradleybuda
Obviously crazy advice that the current generation of Javascript programmers
was wise to disregard. /s

~~~
idreyn
Well, yeah. You have one thread to work with in the browser, and if you tie it
up with a synchronous XHR your application freezes until the request finishes.

------
seanwilson
I've seen something like this before that allowed login with any password as
long as you gave a valid user name:

> if (hash(password == hashedPassword)) { login(username); }

"Don't write your own hash function" is common advice, but I would go much
further and say don't write anything to do with passwords, logins, roles or
sessions etc. if you can because one small mistake is all it takes to create a
huge security hole.

~~~
jedimastert
Does anyone have any experience with password-less login? Like were you just
do the other half of a 2fa like an email? And I mean both experience as a
consumer or developer.

I feel like if I were to have a service that required log in that's how I
would do it, but with all the talking about it I've never actually seen it in
the wild.

~~~
gen3
This is how Hack Club Bank works. You enter your email, then wait for the 6
digit code emailed to you. No passwords at all.

------
banana_giraffe
According to a reddit thread [1] from a few years ago, this is from an
internal application, though on a public facing server.

Still, I agree with one of the posts on reddit, this is a decent ice breaker
interview question to see how many problems the candidate can find.

[1]
[https://www.reddit.com/r/programminghorror/comments/66klvc/t...](https://www.reddit.com/r/programminghorror/comments/66klvc/this_javascript_code_powers_a_1500_user_intranet/)

------
8lall0
This is genuinely stupid code from a first-week-junior dev who is still
learning a lot.

The worst code i've ever seen was a PHP function that printed a menu (with
relative permissions) from a database. Problem: it was O(n^3), 200 lines long,
completely unmantainable and with random fails. Problem n° 2: it was written
by our "best" programmer. It took me one hour to rewrite that into a 10 lines
O(nlogn) function that my PM didn't want to use inside the framework because
it was my first week.

~~~
techslave
i hope you left before the 2nd week!

------
jarym
But the employer wanted a full-stack developer, everything else is just
details!

I’ve seen worse code than this. How about a hard coded backdoor check that
looked like: || pw == “debug0”

~~~
mercer
Hmm. I have hard-coded superusers for an app that are based on email address
(where email is the login username). I'd love to hear from the HN experts if
that is a very bad idea and how to do it better.

(initially I had a 'superuser' permission that would sort of short-circuit the
permission system, but I felt really uncomfortably about how much that
affected my code. I figured having compile-time hard-coded superusers per-app-
instance would make more sense).

~~~
jarym
I think its a bad idea to hard-code anything like that.

If you have to do it then accept it as a command-line argument or read it from
a config file - at least then it can be backed-out with an application restart
instead of a recompile.

~~~
jarym
Oh and it also becomes more obvious for people who may not have access to the
source code that its there!

------
jl2718
But the dev made 15 commits this week and knocked out 5 Trello tasks. All
tests pass. Top performer.

------
al2o3cr
TBH if this is the "worst code you've ever seen", bless you sweet summer
child.

~~~
runawaybottle
Right? I suppose he/she is focusing on the naive authentication piece, but I
honestly thought most of the code was clean and sensible (the dev is just
inexperienced). I’d rather teach this person the technology than someone who
constructs labyrinth-like mental models, but knows the technology. They are
code terror incarnate.

There are many people that could take that simple function, split it across
three files, with various events being dispatched, with “elegant” switch
cases, and over abstracted utility functions all over the place. These are the
minds I fear, these monsters will eat your sanity with their contraptions.

You know nothing Jon Snow, this code is good. I know what the person was
trying to do, and it can mostly be solved. Please purchase a ticket to a few
React apps, or a home grown php framework. Then we’ll talk. You have not met a
real mind eater yet.

------
toomanybeersies
> Here we can see the use of the double quotes for writing strings ... Here we
> can see single quotes ... This may not look important, but it’s actually
> telling us that the developer has probably copied some code from
> StackOverflow without even rewriting it following a common style guide for
> the whole codebase.

I have a habit of mixing double and single quotes, depending on what side of
bed I woke up on. Doesn't mean I copied my code from Stackoverflow.

~~~
ziml77
Same. It's very easy to mix them, sometimes on lines entered minutes apart.
I'm used to double quotes in C#, C++, Rust, etc. so it's just my natural
fallback when coding in Python. I find myself doing it with SQL occasionally
too, but there it's quite obvious right after entering the double quote since
the string syntax highlighting doesn't kick in.

~~~
mark-r
I don't seem to have a problem with context switching. I use C++ 90% of the
time, but in Python I use single quotes just because I'm too lazy to use the
Shift key.

------
jedimastert
Does anyone have any experience with password-less login? Like were you just
do the other half of a 2fa like an email? And I mean both experience as a
consumer or developer.

I feel like if I were to make a service that required log in that's how I
would do it, but with all the talking about it I've never actually seen it in
the wild.

~~~
jackewiehose
Discord is using user/password but every time I want to log in it says my IP-
adress has changed and I have to use my email to verify my location. So it's
basically the same as password-less email-login.

That's terribly annoying to me as a consumer so please don't do it.

~~~
Can_Not
I lost a discord account by forgetting to change my email address when
sunsetting a domain then getting a new phone.

------
mrbonner
Not the worst, though. It maybe insecure because the app was intended for
internal use.

I did, however, see some shitty stuff in my job. Like, someone iterates a
hashtable to find a key. I didn’t even know what to explain to that person
when I reviewed the code.

------
aljgz
This is a piece of jewelry, compared to what I've seen.

Our team was called in to help on a strange case. An important organization
had corruption in the management and they had contracted a mission-critical
service, one that created income, to someone.

The code was written in asp (the project has started in 2016, 16 years after
the last release). Database tables where named table_01 to table_36, columns
named Column_01 to whatever. All values stored in database were "encrypted"
using base64 most files where 10,000+ lines of repeated code, no modularity,
no naming convention, not DRY.

All the income from the orders went to the programmer's personal bank account.
The money would then be wired to the right companies.

Well, this was just the beginning, the code was much worse, I cannot bring any
examples as I don't have the code and don't remember, but suffice to say that
even though we had made our minds that we're going to handle this mess, but
faced much much worse problem than we expected.

We came up with a smooth plan to transition to a good state.

Meanwhile, the corrupt management had been lobbying, got reinstated, made the
version 2 contract with the same guy and the team I consulted was asked to
just keep the version 1 running while the genius is coming up with more
brilliant ways to write version 2 of pure shit.

------
kanobo
I'm sure most of us have seen far worse and more nonsensical code, it's a
nicely designed snippet to teach basic security issues and show silly mistakes
though.

------
moltar
I call bs on the code being real, because of triple equal operator. Doubt this
developer would know such thing.

~~~
jschwartzi
Every JS IDE warns you about that so I could see a developer correcting the
warning.

~~~
wonderlg
Do you think this dev uses an IDE?

------
rawfan
I just saw an atrocious piece of Perl code that controls the flow of shipping
containers for several large corporations. It’s completely unmaintainable and
only the original author has the complete picture.

He was originally a trucker on office duty because of health issues. In the
office he saw lots of inefficient thing so he bought a copy of „Perl for
Dummies“ and started fixing.

The product itself is actually a huge success. No developer is willing to
stay, though, so the guy just picks other people from the office and teaches
them what he thinks is Perl.

------
Seb-C
I have seen way worse than this.

In a "corporate [bull]shit" setting, I could easily imagine scenarios where
the synchronous client-side query could make sense and be the best available
option. Thankfully I have escaped this kind of workplace long ago.

The worse codebases IMHO are the ones that have random associative arrays that
are exchanged and mutated randomly in any part of the code. Give it a few
years of maintenance by bad developers, and you end up with a mess of
conditional workarounds and variables that can contain up to a dozen of
different data structures with inconsistent properties.

------
riffraff
I swear, I conducted an interview once where the candidate actually did this
(tho it was server side).

They could write fizzbuzz tho.

------
_ZeD_
I can only suggest you to spend some time on thedailywtf.com ... it can be
frustating as much as hilarious

------
fao_
Hold up.

The expectation of junior developers seems pretty low. I think I've been job
seeking below my mark...

~~~
alephu5
Yes, this is beyond incompetent. Even someone without any programming
knowledge would probably know that you can't have an API for checking the
username and password combinations of all users.

------
29athrowaway
1\. SQL injection as a service.

2\. It should implement this logic server-side.

3\. No hashing + salting of passwords.

4\. It should retrieve one user at a time, rather than all of them.

5\. Not understanding basic control flow.

...

There was probably a presentation about this project, where the author
received a round of applause. This person was likely promoted for finishing
this project in record-time.

The developer that spent time reporting the issue lowered their own
performance metrics in exchange for a bug report that was given a low
priority. When the developer objected to the prioritization, the project
leadership got pissed off and punished the developer evaluation citing reasons
such as "does not align with business needs", "has poor communication skills"
(talks abstract things that nobody understands or cares about).

By the time this defect was found and fixed, the author likely pushed 10
different other defects just like this. Also, other engineers copied and
pasted this code multiple times because it's working and has unit tests.

~~~
unsignedchar
Were you going for a comic effect or tragic?

~~~
29athrowaway
Is Office Space a comedy or a documentary?

~~~
eckza
... yes.

------
shusson
Anyone else find the quotation marks in the original picture suspicious?

Especially around:

    
    
      if ("true" === "true") {
        return false;
      }

------
ConcernedCoder
This can't be real, it seems to be especially crafted to break most if not all
common-sense programming rules...

------
esgwpl
I like how everyone's answering questions about someone's personal experience
with even more personal experience, I'm not being sarcastic, keep them coming,
please.

------
kaetemi
"Just throw more hardware at it."

------
rl3
> _... why they’re not hashing passwords inside of their database?_

Sending passwords plaintext over the wire isn't good either, even with TLS.
You don't want your server to have any knowledge of the plaintext password.
For that reason, it's a good idea to salt and hash passwords client-side as
well.

An even better idea is to not roll your own authentication if you can help it.

~~~
thaumasiotes
> Sending passwords plaintext over the wire isn't good either, even with TLS.
> You don't want your server to have any knowledge of the plaintext password.

This is literally impossible to avoid. Whatever the server receives, that's
what the password is.

~~~
ilammy
This is literally untrue. Zero-knowledge comparison protocols exist [1], and
their goal is to prevent malicious party from obtaining any knowledge of the
secret. Yes, the _genuine_ server will have its copy of the password (or its
hash, whatever) and the user will have their copy of the password too. What
you don't want is to have the user send their copy of the password to a place
which looks like your server but isn't.

[1]: [https://www.cossacklabs.com/files/secure-comparator-paper-
re...](https://www.cossacklabs.com/files/secure-comparator-paper-rev12.pdf)

EDIT: I mean, yeah, you're right in the sense that whatever you send to the
server is used to authenticate you, so that whatever is a 'password' of a sort
by definition. However, using the raw password as is for authentication is not
a great idea. If you intercept the password, you will be able to compromise
any further authentications. There are alternatives that avoid this.

~~~
DarthGhandi
Another alternative is a PAKE. You'll never have a password sent over the
wire.

For some reason PAKE schemes aren't that commonly used despite having a solid
grounding. Matthew Green has a few good articles on them.

~~~
thaumasiotes
> You'll never have a password sent over the wire.

As far as I can see, you do need to send the full password over the wire once
when you set its value. But the server doesn't need to remember it.

> For some reason PAKE schemes aren't that commonly used despite having a
> solid grounding.

It doesn't look like something you can implement unilaterally. How's browser
support?

(Interestingly, I guess you could implement it unilaterally between your
server and client-side javascript...)

~~~
DarthGhandi
I'm not sure if this is coming down to semantics here but the password is
essentially encrypted to everyone except the client, I wouldn't call that
sending a password over the wire, you may disagree. This is a unique value for
every login attempt.

Probably the most common scheme is SRP.

[https://en.wikipedia.org/wiki/Secure_Remote_Password_protoco...](https://en.wikipedia.org/wiki/Secure_Remote_Password_protocol)

~~~
thaumasiotes
No, you're right; I wasn't talking about login attempts. I was thinking that
PAKE is a way to establish that two parties have common knowledge of a shared
secret, and so the password needs to be shared once in order to become a
shared secret. But the protocol you link doesn't transfer the password even
when setting it.

(Weirdly, the article explicitly notes that s, the salt Carol applies to her
own password, is shared with the server and sent over the wire during login.
But the salt never appears to be used by the server in any capacity.

And the instruction "Carol must not share x with anybody, and must safely
erase it at this step, because it is equivalent to the plaintext password p"
doesn't make a lot of sense; presumably Carol isn't going to erase her
knowledge of her password -- what does erasing x add?

[The only thing making the password equivalent to x is that the server
transmits the value s during the login attempt. If it didn't do that, x would
still be useful, but the "password" would be worthless, unless of course s was
stored alongside the password...])

------
nerder92
Honest question, is code a very important part of our job as a developers,
seniors or junriors? I truly think about code as an implementation details, I
don't really care how the code looks as soon as is quality tested (and cross-
functionally testes, ie: security). I agree with the sense of the article in
general, this code lacks basic engineering/programming concepts, but I
generally disagree on the fact that code as a piece of text that execute a
software have any meaning of "beauty" standard as such. Code is just the way
we have in this specific moment in time to describe the solution of a problem,
very far from being the only one ore the "final" one. Therefore I would not
invest much time in putting lipstick on cows.

~~~
kingdomcome50
In short. Yes. Most code needs to be maintained and understood(!), so having
some semblance of standardization (in its many forms) usually provides
benefits to the stakeholders.

And to address your last sentence, I think it’s important to understand that
for a skilled engineer it _does not necessarily_ take more time to write
better code!

~~~
nerder92
I totally agree with you on the fact that for a skilled engineer good code
does not comes with a bigger cognitive load (nor time spent). But I would like
to challenge your point on writing maintainable and understandable code. One
can argue that writing maintainable code following the best practices it's
just a set of conventions that are useful just because we have no better
solution right now. It's a solution that works but can be very well be
improved for instance by removing the needs of writing code in the first place
(ie: with some very advanced level of code generation). About the
understanding bit I usually look at tests for understanding code, it's easier
for me to read an English statement that tells me what a things does rather
then trying to understand it starting from how the problem has been solved in
details. Code is for machines, "it should do this" is for humans.

~~~
kingdomcome50
Surely one must actually improve upon the current conventions before
dismissing them, no?

Similarly the field is called computer _science_. Not every convention an
engineer follows is simply because "we have yet to find a better one". I can
certainly agree that the landscape has and will continue to change over time,
but there are some truths when it comes to designing systems.

As for gleaning understanding, tests are useful for learning "what" a system
is meant to do, but wholly meaningless when we need to know "how". Often the
latter becomes the important bit when we need to extend/modify code.

I agree that there is a balance to be had. Not every LoC or module needs to be
perfect. It's often more important (and practical) to choose a suitable level
of abstraction, and make sure that the code _at that level_ has an amount of
agreed upon standardization.

