|
I remember there used to be a thread like this, but I can't find it anymore. Just post code that either you or your coworkers/classmates have written that is hilariously bad. I've been looking through a large system recently, since I just started a new co-op and I have to get caught up. This one coder (who I've never met) has stuff like this in all his classes: private static final int FIVE = 5; private static final int NEG_ONE = -1; private static final int FIVE_BILLION = 5000000000; private static final String SPACE_PIPE_EQUALS = " |="; private static final String ERROR_WITH_CODE = "Error with code"; I was reading one of his classes today that had 50 lines of constants written like that. In an 800 line class. Also, in multiple places he does this: File fw = new File(args[0]); File fh = new File(args[1]); File nv = new File(args[2]); File nh = new File(args[THREE]); File tw = new File(args[FOUR]); He does this in multiple classes. 0, 1, 2 are fine, but any number higher than that he replaces with a named constant. He also doesn't comment his code, and uses method names like "task" and "task2". Then there was this method: code:
I'm sure compared to many of you that this isn't all that bad, so share your own tale of horror!
|
# ? Mar 21, 2008 00:33 |
|
|
# ? May 4, 2024 12:16 |
|
How come the guy is still employed?
|
# ? Mar 21, 2008 00:51 |
|
Don't you know the virtue of avoid magic numbers in your code?
|
# ? Mar 21, 2008 01:03 |
|
CeciPipePasPipe posted:How come the guy is still employed? My guess is that he is/was a co-op. They were apparently rather excited to hire me, and all I did was answer some pretty basic questions on OOP (not saying this to brag, because I am not that good of a programmer), so they probably don't have the most talented pool to choose from. Hell, they're hiring from my school, I know they don't have that talented a pool to choose from. When I mentioned the lovely naming to the senior developer, he said he had not seen it, so probably no one really noticed.
|
# ? Mar 21, 2008 01:14 |
|
code:
|
# ? Mar 21, 2008 03:22 |
|
I worked at a place that used SQR that i had to convert to PERL. The SQR code was used to output reports and random information from the sybase server. It was some of the worst code I have ever seen. The code had methods that were in no particular order and the main method was spread throughout the document. The developer used no variable scope (A variable would be declared in one method and used in a different one without passing it). To make things worse there was embedded sql that would query entire tables where only a few fields were needed. Don't ever agree to learn SQR! It was the worst decision of my life. Oh and this isn't a coding issue but the code was separated into different files (an attempt at code reuse), but the files were spread across three different version control servers and there was no way to tell if you were working on the most current version.
|
# ? Mar 21, 2008 05:15 |
|
code:
code:
|
# ? Mar 21, 2008 06:40 |
|
very posted:Don't you know the virtue of avoid magic numbers in your code? Are you being sarcastic? Please please please be being sarcastic. const int THREE = 3; is just as bad as using 3 everywhere. It should be const int NUM_CHOICES = 3; or whatever 3 represents.
|
# ? Mar 21, 2008 16:31 |
|
But THREE is more descriptive....
|
# ? Mar 21, 2008 18:07 |
|
From a Rails application:code:
|
# ? Mar 21, 2008 18:09 |
|
Ranma4703 posted:
code:
|
# ? Mar 21, 2008 18:26 |
|
ashgromnies posted:Are you being sarcastic? Please please please be being sarcastic. Haha. Yeah.
|
# ? Mar 21, 2008 18:37 |
|
dwazegek posted:I have a colleague who also does stuff like this. For whatever reason, he refuses to use return anywhere, except the last line of a method. This frequently results in methods with absurd amounts of indentation, or stuff like this: It's an application of the Single Function Exit Point philosophy, with which I don't agree. I think it's good for some issues, especially resource management, but largely you want to use RAII for that anyway, and code can get very messy if you can't bail out early (goto, anyone?).
|
# ? Mar 21, 2008 18:50 |
|
dwazegek posted:
This would be perfectly fine if he had ever heard of the "break" keyword
|
# ? Mar 21, 2008 18:52 |
|
I can't post it because I don't have access to it any more, but I was recently contracted to do an audit on the code of a custom mailing list program after the company doing it said (after a year of "development") that MySQL could only handle around 50,000 emails and that to handle the 450,000 expected ones they would have to upgrade the server to Oracle. The program itself was out of my league so I subbed it out to a friend who is an amazing programmer and he would send me examples of the security like "12345" type passwords and completely illegal and actionable stuff like the inability to opt out of the list. Other things he found going through it: -database password was wrong -No error checking on mysql_connect, mysql_query, or much of anywhere actually. -Opt out wasn't just broken, it just didnt' work. "when it displays the results of the filter/search it omits the people marked as opted-out. Buuuuuuut, when it processes to send email it doesn't." This is how my friend described the entire project: "The site is like....a kid knows his multiplication tables, but that doesn't mean he knows how to apply that to solving applied calculus. The guy that did this site knows that nuts fit on bolts, but can't build a truss." I have been cleaning up after this development company and their big product is this all encompassing content management system that would take someone three months to learn how to create learning PHP/MySQL from scratch.
|
# ? Mar 21, 2008 19:37 |
|
dwazegek posted:
Every kid knows that false || x equals to x (in a boolean context) He could have mine as well written that line as code:
|
# ? Mar 21, 2008 20:08 |
|
Bonus posted:Also what the hell is up with setting a variable to false and then doing var = var || x Some point along the way, retVal will become true (found a valid item), so it should stay true. Of course, a sane person would just say "if(item.isValid) return true;" but that's neither here nor there. Also my favorite is still code:
|
# ? Mar 21, 2008 20:12 |
|
Oh yeah, drat, missed that. That's exactly why you shouldn't try to be clever in your code and just use the most readable and clear code possible (in this case, an if statement)
|
# ? Mar 21, 2008 20:15 |
|
Avenging Dentist posted:Also my favorite is still I've done that occasionally for writing test code (along with "#define class struct" ) but, yeah, finding it in something that's about to ship always amusing.
|
# ? Mar 21, 2008 20:22 |
|
Bonus posted:Also what the hell is up with setting a variable to false and then doing var = var || x Anyway, those two examples are pretty old, probably from when he first started working here, and seeing as I cringe at my old code as well, I can't really blame him. I found this abortion in a project I wrote about a year ago (and even then, I should have known better): code:
|
# ? Mar 21, 2008 20:24 |
|
I've got two PL/SQL ones that I found recently. First, I found this:code:
Also, this gem was found by a coworker and while it doesn't look terrible, it led to a ridiculous bug: code:
Of course, since userid can't be found in the table, and it wasn't explicitly referenced as users.userid, Oracle is helpful and assumes you mean the lexically scoped delete_user.userid, turning that condition into WHERE delete_users.userid = delete_users.userid. So we've set is_deleted on every single user! And then it loving commits! Jesus gently caress people, do not ever put commit in your procedure. npe fucked around with this message at 21:52 on Mar 21, 2008 |
# ? Mar 21, 2008 20:54 |
|
Avenging Dentist posted:Also my favorite is still Haha, that reminds me of some code that was found in a header of some middleware, which shall remain nameless code:
|
# ? Mar 21, 2008 20:58 |
|
code:
I remember a long time ago seeing something similar on theDailyWTF... code:
|
# ? Mar 21, 2008 21:20 |
|
While I've got nothing as bad as that which has been posted, a colleague of mine was looking through some of my code and found something equivalent to this gem:code:
|
# ? Mar 21, 2008 21:22 |
|
Avenging Dentist posted:Some point along the way, retVal will become true (found a valid item), so it should stay true. Of course, a sane person would just say "if(item.isValid) return true;" but that's neither here nor there. code:
|
# ? Mar 21, 2008 21:39 |
|
csammis posted:This would be perfectly fine if he had ever heard of the "break" keyword True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios. My favorite WTF come from our HTML types who don't get simple concepts like "don't repeat yourself", "non-destructive testing" and "find and replace is not a valid maintenance technique."
|
# ? Mar 22, 2008 00:59 |
|
wwb posted:True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios. Because it's an archaic overreaction to spaghetti code. It should be used only when doing so would make the code most clearly understandable.
|
# ? Mar 22, 2008 01:21 |
|
wwb posted:True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios. One of my gripes with it is the "retVal" variable (it always seems to be named that way), in the vast majority of the cases, the first "real" value that is assigned to it, is the value that it's going to have at the end of the method. So why not return the value immediately? Personally, I prefer returning the value immediately, because it makes reading the code easier (or at least, easier for me); if I see a branching statement that immediately returns a value, then I know that the method is done if that branch is hit. If you go with the "single point of return" method, then this is less clear.
|
# ? Mar 22, 2008 01:44 |
|
floWenoL posted:Because it's an archaic overreaction to spaghetti code. It should be used only when doing so would make the code most clearly understandable. dwazegek posted:the "retVal" variable (it always seems to be named that way) Incoherence fucked around with this message at 01:51 on Mar 22, 2008 |
# ? Mar 22, 2008 01:49 |
|
Incoherence posted:But at the same time, not using it ever is an overreaction. If it makes sense, use it. If it doesn't, don't use it. Imagine methods with 10+ levels of indentation and stuff like this appearing regularly: code:
Incoherence posted:Personally when I write a IsFooCondition() that calls for such a variable, I feel stupid calling the variable some variant of isFooCondition. dwazegek fucked around with this message at 01:56 on Mar 22, 2008 |
# ? Mar 22, 2008 01:54 |
|
dwazegek posted:I'm partial to the almost always applicable "result" See, where you might use "result" I would use bln_flg_tst_cnd_args_x.
|
# ? Mar 22, 2008 02:01 |
|
tef posted:See, where you might use "result" I would use bln_flg_tst_cnd_args_x. Hahahahahahahahahahaha Oh COBOL, you humor me so.
|
# ? Mar 22, 2008 02:48 |
|
tef posted:See, where you might use "result" I would use bln_flg_tst_cnd_args_x.
|
# ? Mar 22, 2008 02:49 |
|
return0 posted:While I've got nothing as bad as that which has been posted, a colleague of mine was looking through some of my code and found something equivalent to this gem: What's wrong with that code? Is it just that code:
|
# ? Mar 22, 2008 04:03 |
|
such a nice boy posted:What's wrong with that code? Is it just that That's not exactly the same... he would need something like code:
|
# ? Mar 22, 2008 04:19 |
|
Never has a single empty method call made me want to hit something so badly.code:
|
# ? Mar 22, 2008 06:08 |
|
tef posted:See, where you might use "result" I would use bln_flg_tst_cnd_args_x.
|
# ? Mar 22, 2008 06:16 |
|
dwazegek posted:One of my gripes with it is the "retVal" variable (it always seems to be named that way), in the vast majority of the cases, the first "real" value that is assigned to it, is the value that it's going to have at the end of the method. So why not return the value immediately? RetVal is useful if you're caching outputs somehow like in a map. Since many cache implementations can asynchronously dump some data between testing for its presence and actually getting the data itself, a common idiom is to get the item in question and then test for nullity. If it's there, we got a value. If it's not, we got null and need to switch to Plan B.
|
# ? Mar 22, 2008 06:32 |
|
Mr. Heavy posted:Never has a single empty method call made me want to hit something so badly. Fuzzy logic!
|
# ? Mar 22, 2008 06:49 |
|
|
# ? May 4, 2024 12:16 |
|
Mr. Heavy posted:Never has a single empty method call made me want to hit something so badly. I see someone has been reading Schrödinger's book on programming.
|
# ? Mar 22, 2008 13:25 |