- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
An actual happy ending （ ﾟДﾟ）
Admin
I think the "return true" was better, after all.
Admin
It may be a pitifully bad algorithm, but major props for clarity of variable naming, consistency of structure, and even making the easy validations ahead of the harder ones.
This is bad in the sense of "I've been handed a task I don't really understand", not in the "I have no business using a computer for anything except watching youtube." sense we see so much of around here.
Admin
When you are new, sometimes the right approach IS to do what you know, and learn from it as you go later. It's lacking that second part that often causes the WTFs, not doing the first part.
Admin
Happy ending is TRWTF.
Admin
Indeed, and it seems that Patricia has learned lessons, in the technological and human domains, from her experience, and learned them well.
Admin
So TRWTF is lead developer blindly trusting interns?
Anyway, the best part for me is the comment " //Checando tamanho do numero" (translation: checking number length, right before "cnpj_a.Length != 14". Redundant comment is redundant.
Addendum 2019-01-23 08:10: Actually, TRWTF is expecting code review in Brazil. The companies here just don't do that.
Admin
IF the code works its not really a WTF at all. Sure its not very pretty at all, but for an intern its a good job. We all started somewhere, I occasionally look at some code I wrote more than 30 years ago for a product that won awards (but failed commercially, competing with Microsoft back then was mostly impossible) that also worked, and it's cringe worthy compared to what I do today. Of course we started when our C compiler did not even have prototype functions. I would rather feature WTFs written by people who should know better.
Admin
As a Brazilian also studying CS, although the task is done, the code is ugly. I don't know for how long this intern has been studying but for me TRWTF is how a lot of people in advanced stages of the course still code like this
Admin
What's exactly wrong with
if (cond) foo = false. You can't just nillywilly sayfoo=!condwithout altering the algorithm...Admin
TRWTF is that there are countless libraries that perform this validation and people insist on reimplementing it.
Admin
People are constantly trying to reinvent the wheel, hoping they come up with some kind of tractor belt, but usually just making a mess. I'm sure a lot of them think, "Well, somebody had to program those library functions. It must be 'doable' so lemme give it a try...."
Admin
If they don't know a for-loop what makes you think they know about libraries?
Admin
Brilliant Paula Bean? She's Brillant.
Admin
I think a lot of contributors here don't seem to get the fact that it's immaterial whether the code works or not, or whether it's efficient or even pretty.
TRWTF is simply:
“Nah, just go ahead and merge, I’m sure it’s fine.”
But then that's the whole point of the article, really.
Admin
"Anyway, the best part for me is the comment " //Checando tamanho do numero" (translation: checking number length, right before "cnpj_a.Length != 14". Redundant comment is redundant."
Not redundant at all.
The three comments are actually good comments -- they give the top-level indication of what the checks are being done:
Check the length is correct
Check the first checksum
Check the second checksum.
A perfect encapsulation (very probably) of the specification that the intern was probably working to.
Admin
If it's stupid and it works, it's not stupid.
Admin
Not only is this implementation clean and clear, but I rather doubt the CPU savings involved in calling a library function or rewriting the code to vectorize, or install for-loops, is measurable. Unless of course the parent application is going to validate 1E8 of these codes on every call.
Admin
"nobody likes it when you if (someCondition) foo=False". Yeah, but "foo &&= someCondition" isn't very pretty either. "foo = someCondition" would be wrong though.
Admin
"Did we write it?" "No" "Then it can't be used"
Admin
TRWTF is getting the chance to go back and rewrite it - if you're not doing anything that customers will pay for why are you being allowed to do it?
Admin
Yeah. I don't like it either. I woulda just had, all one one line: if (cnpj_a.Length != 14) return false; and saved a level of indentation. Comment if any would have been // If it ain't 14 digits, it's bad, so let's bail out of here right away.
Admin
Might be psychological. If you know about a WTF in the code, especially your own code, you think about it again and again. So this blocks your performance. You never reach the flow.
Admin
I'm guessing the test could have been dropped entirely, if the Util methods used on the first line do what I think they do.
Admin
For me, assuming an initial flag value of true for any check and then falsifying the flag is a WTF, because if you miss a condition somewhere the check will likely pass without warning.
Admin
I was half-expecting to see something like:
Admin
The one real problem I see is that there is no try - catch or other check that each character in the original string is a digit. Perhaps there's another input validation before this is called? But I hate to make assumptions like that. Someone will reuse the function without checking all the assumptions, and get upset because someone typed in letters and the validation passed.
Geoff: If you missed a condition somewhere, you'll frequently return the wrong result regardless of whether you first assume it's true or false - or avoid making assumptions by using a string of if ... else if ... else if ... to get all the conditions in.
For the rest:
The implementation of the checksum calculation is ugly, but when each digit gets a different coefficient in no obvious pattern, the only other way I can see is to have a table of the coefficients somewhere and loop through it. That can blow up if the table becomes missing or corrupted. With the coefficients right in the code, the only way it can blow up is if the code is corrupted, and that will foul up any implementation. She could have kept a running sum and used far fewer variables, but I assume that memory won't be critical, and it's easier to debug with all the intermediate steps preserved.
As for "if(cond) foo = false" versus "foo = !cond", it would depend on whether I was confident that the particular language and compiler would always ensure that was 0 or 1.
Admin
This. Junior developers produce shit. Sometimes the shit is flaked with gold, sometimes it is just shit, and its the job of senior developers to point out where and why it is shit, and then rewrite it with the junior in a way that isn't shit. I'm a slightly warped facsimile of the guy who trained me (hi Jon!), and I hope the guys I train now even begin to remotely resemble him.
Admin
Why all the bitching about "if(cond) foo = false" versus "foo = !cond"?
The former only sets foo to false, explicitly, if the condition is true. It won't set foo to true if conf is previously false.
The latter style could potentially change foo from false to true, incorrectly. Maybe not here, but as a pattern, it can.
TRWTF is getting hung up on that stuff
Admin
That is stupid.
Admin
[Soapbox Mode]
On the religious topic of libraries vs roll your own ...
We all agree it'd be silly for GM to make their own nuts & bolts to assemble into their cars. Instead they buy them from established nut & bolt suppliers.
But grabbing libraries more at less at random from npm or getting code snippets off stackoverflow or wherever is about like GM scouring the streets and municipal dumps to find bolts lying around to install into their cars. They'd have no assuraance any of them were well-made, undamaged, etc. It'd be obtaining parts on a "100% as-is, where-is, you assume all risk" basis.
If you can trace the quality of the library code back to established standards and established processes, THEN it's good enough to use. if not, not.
In addition to taking a dependency on the code, you're also taking a dependency on the company behind the code. I've certainly been burned a few times over the years by buying tools for my toolchain or incorporating libraries into my products from companies that later failed, leaving us with a rework disaster.
Open source isn't much better; there are plenty of abandoned open source projects out there. Not to mention the questionable practices of a self-selecting rag tag band of volunteers.
Ultimately your customer will blame you when your product fails them. That the failure is actually from some deeply buried library that some other library included that yet another library included in turn that some other ... is immaterial. You import that mass of uncertainty at your peril.
We will never be an engineering discipline rather than an overgrown collection of cottage handicrafters until we embrace the deep truth that our product is only as good as the crappiest routine inside it. I'll say it again: your (and my) product is no better than it's crappiest include. Don't let randomly selected internet strangers put a ceiling on your work quality.
[/Soapbox Mode]
Admin
On libraries. Is it possible that when the code was written the library was not available?
I do also recall a story where a large lot of code was included simply to get one dictionary definition.