8 Signs your code sucks
As a wide eyed junior developer when I first began working on large projects I simply accepted that it is difficult to fix bugs or find where an action is being executed. If only I knew then what I know now, I would had saved myself hours of frustration. The first step to writing good code is accepting the code you write (or work on) is crap, but sometimes you need to know what to look for. Here are some signs that your code sucks.
A method is larger than the screen – A method should only perform one specific task. A method should not contain the logic code to determine if the username field contains data, is valid, and that user exists. If a method is too large to fit within a single screen, that is a (very) good sign it is doing too much.
You are reusing variables – Unless you are working on embedded devices, memory is cheap. Don't be a memory scrooge and knee cap code maintainability, which in nearly all instances trumps performance, by reusing the same variable for multiple uses.
You are directly accessing the request/session – It not only makes writing unit tests (much) harder, but it is also difficult to know what data the application has access to. All data should be taken out of the session/request and stored in a bean. A bean, through its getters and setters, creates a “contract” of what data the application has access to, which greatly helps with code maintainability.
You need to use comments to explain the code – Code should be able to explain itself and should be in a format that is easily readable. If you find yourself needing to explain what your code is doing then you may want to look into rewriting that code.
EDIT: This is not referring to using comments (e.g. javadoc) to explain the purpose of a method/class and its inputs and outputs.
An exception's stack trace doesn't return the original problem - You should never “eat” an exception, that is catch an exception, but not print its stack trace. How can a bug be fixed if you don't even know where the bug is occurring?
Your code is a mud ball – Just the name sounds ugly. A “mud ball” is when there is little separation between the layers or concerns of an application. Code should be modular allowing for ease of reusability and modifiability. Anything concerning the user interface happens in the view, program flow and usually data validation is the domain of the controller, handling business logic is the model's job, and only the model should be interacting with the data access layer.
It is hard to write a unit test – If you find a bug or write a new piece of code and it takes you more than a few minutes to write a unit test then that portion of code is handling too complex of a task.
The author is Billy Korando – Wait what?!
Add your own experiences or signs for dealing with sucky code.
UPDATE: There are differing opinions on my signs, since I am but a mere "grasshopper" I maybe wrong. Anyways check it out and make up your own mind.
Related posts:
Categories
Tag Cloud
Archives
- November 2009 (3)
- October 2009 (18)
- September 2009 (15)
October 22nd, 2009 - 08:02
A good book to check out for other signs your code sucks, or code smells as he refers to them, is Martin Fowlers excellent book Refactoring.
October 22nd, 2009 - 10:07
Comments are good – mmmkay?
I would rather have comments than no comments, but they should add value. Sometimes a complex problem requires complex logic to solve it.
That said, many of my comments address *why* I did something the way I did. Readable code is good, but sometimes even I go back to code and wonder why I did something a certain way. Sometimes I need to do something because of the third party lib I am working with requires something non-intuitive. Some of my comments are TO-DOs (yes, I do go back and address them).
There are all kinds of code smells.
Copy/paste coding is one (the code should be refactored and reused, not just pasted a bunch of different places) – that may seem obvious, but even today I see a lot of copy/paste coding done by devs who know better and earn 6 figure salaries. Take a look at java.awt.datatransfer.MimeType and javax.activation.MimeType in the JDK source code – with a few lines difference, someone at Sun copy/pasted that whole class. But the more common case is maybe 30 lines of code from one method, or a couple of methods, then they are tweaked a little. Lazy sloppy coding. And yes, the people who copy/paste are often the same people who can’t break up their code into modules methods – I run my 24″ monitors in portrait mode and I still see methods that run more than the full screen (100 or more lines of code in a method).
October 22nd, 2009 - 16:16
Agreed, some people just add comments to be agreeable and a good citizen.
To say ‘your code sucks’ if you try to throw the reader a bone is ludicrous. Copy-paste on the other hand is craptacular.
October 22nd, 2009 - 17:32
You maybe a good citizen by commenting your code so other can understand it, but you would be a great developer if you didn’t need to comment your code.
You are absolutely right though, copy/paste code is craptacular.
October 24th, 2009 - 04:05
I think that the “great code explains itself” is, in the end, impossile and just plain stupid
Try returning to some old code in half a year or a couple of years even, and notice how the comments *do* make things nicer.
October 24th, 2009 - 10:17
I’ve picked up great code written by other developers that had no comments, and I easily understood exactly how it worked. As developers it is our responsibility to learn how to read code.
The problem with comments is that as the code grows and matures, they are not kept up to date. How many times have you seen:
/**
* @param name
* @param manager
* @param dept
*/
public void gargleTheFlooble(String name, String manager) {
…
}
Really helpful comments, those.
October 22nd, 2009 - 16:18
Yeah I disagree with #4. On large projects, sensible comments and method docs are a must.
October 22nd, 2009 - 17:28
It wasn’t in reference to javadocing, or its equivalent in other languages, of methods/classes. I was referring to having a crazy piece of code that have poorly named variables, overly complex if statements, or a really deep hierarchy that would require some sort of explanation to understand. Code like that isn’t acceptable and should be refactored.
October 22nd, 2009 - 18:43
I comment my own code to bits, not because my script doesn’t explain itself but many times my brain is tired and reading a reminder or explaination helps those mornings (like now), when my hard working brain is … errh can’t think!
October 22nd, 2009 - 19:37
I think most of these are suitable for smaller development teams and smaller projects. Taken to an extreme, some of these could be seen as bad advice. Any development team of reasonable size is going to have A, B, and C-level developers. You’re not writing comments, for example, for the A and B tier developers – you’re writing them so the likelihood of someone else (i.e. a C level developer) coming along and doing something wrong because they don’t understand the code as well as others do.
Similarly, while I agree with the sentiment of not trading terseness for memory usage, if you’re dealing with very large scale systems (and scaling them affordably), memory is absolutely a very big deal.
As with anything, with experience you learn the trade-offs between both ends of the spectrum on any of these things
Good list though!
October 22nd, 2009 - 19:39
One other comment – the gem in this list is “it is hard to write a unit test”. When coming into a new codebase, one of the first things I like to do is try to write a unit test (or sit with a another developer who is). Watching how hard it is, and the techniques used in existing tests, can point to many areas for improvement and refactoring.
October 22nd, 2009 - 21:29
>You are reusing variables – Unless you are working on embedded devices, memory
>is cheap. Don’t be a memory scrooge and knee cap code maintainability, which in
>nearly all instances trumps performance, by reusing the same variable for multiple
>uses.
Uhm… now this is just silly. You think that reusing variables save memory??? oook… you aren’t talking about assembly, are you?
October 22nd, 2009 - 22:22
How would it not? If I declare two int variables in the same scope that would use more memory than declaring a single one and reassigning it. The memory savings is obviously insignificant which is why I recommend against the practice.
My experience is limited to high level languages, but I can’t imagine where declaring multiple variables would have the same memory impact as declaring a single one. The values to those variables HAVE to be stored somewhere.
October 22nd, 2009 - 23:39
A good compiler may be aware that ‘int foo’ isn’t used after a point, and ‘int bar’ is only used after that point, and so assign them to the same memory location with just a reinitialisation in between. I don’t know if that happens in practice, but it could be.
October 23rd, 2009 - 03:32
You are reusing variables
This is good in some cases, e.g. in graphics programming while working with big pixel arrays, histograms and stuff like that.
You are directly accessing the request/session
There are solutions to mock requests and sessions for unittests.
You need to use comments to explain the code
Comments are good in case the code is some complex algorithm.
October 23rd, 2009 - 08:23
I know you can mock a session/request in unit tests, but it makes it MUCH harder. Save yourself the headache and write a bean to hold that data.
Also like I said in my new post further explaining my opinion on comments, these signs are guidelines. There is going to be the rare cases when you need to ignore them, but that shouldn’t be your modus operindi.
October 23rd, 2009 - 08:44
To be honest, I was very low hopes for a post with a headline of “X signs your Something Something”, but this was short and sweet. Eating/swallowing exceptions is my *biggest* pet peeve. And writing testable code is a bit struggle to new developers in particular. If you haven’t been exposed to much testing, it’s difficult to understand how to write testable code.
Great post.
October 23rd, 2009 - 12:20
Good comments explain the why, not the what. If you need comments to explain what, ur junior anyway.
October 23rd, 2009 - 12:27
Using separate variables does not take up any more memory than reusing the same variable over and over.
October 23rd, 2009 - 16:12
…
Did you think about this?
There are trade offs, generally you can optimize for speed or memory (thinking as a C programmer). When these issues aren’t critical requirements the features of other languages become more attractive. You can write any recursive solution as an iterative but generally in doing so you are trading memory for speed (typically such a solution requires a lot of stack manipulation). This seems java oriented but if you’re being picky it not everyone is using a garbage collecting compiler/language there is overhead in doing so and unacceptable unpredictability for some requirements. Using the same variable over and over is just confusing in most cases, what are you going to call such a variable, x I suppose?
I used to have a big interest in graphics where even the slightest inefficiency in vertex transformations becomes a big deal. Graphics, at least the kind that are interesting are pushing the limits of machines you can greatly cut down performance by saying “Oh I’ll let the language/compiler handle that for me”.
The OP was talking about every day business application programming and I think he makes perfect sense to all but those who want to be obtuse about matters. Why does every one who writes an advice piece get people telling them they didn’t take care of all the exceptional situations. Happens in every damn post where you let people comment. I mean if they wrote “You know I agree and I also feel … is important.” Wouldn’t that be nice but no they have to say “Oh look you forgot (or are wrong) … (some stupidly specific situation for which the really general advice was not intended or he would have needed to write a book you [expletive removed])”.
October 23rd, 2009 - 16:22
Thank you! Somebody finally gets it!
October 23rd, 2009 - 18:50
With PHP it’s almost an absolute must when working with large amounts of data to sometimes reuse variables. There are ways around it, but PHP’s memory management blows.
October 24th, 2009 - 02:20
Most of the tips are true, but sometimes trying to gain speed we forget about OOP and make our code close to procedural.
October 24th, 2009 - 18:45
I have to say I disagree with this article. I feel its hostile. Instead of saying how to make your code better you just said how it can suck, not much substance. I would always strongly advocate to every developer and my students to use comments as much as humanly possible. Its helpful to just note the why, what the function does if your ending nested if else statements its nice with the indentation to also make a quick note like // end if summer or whatever. I feel that people who don’t use comments must spend half there time re-reading scripts during revision stages again and again because they have no notes. I can see the point your trying to get at, that is that if the line/section of code is really complicated that it needs alot of commenting to explain that maybe it needs to be approached from a different direction.
When I teach people new to coding I tell them three things. Comment your code, approach your idea from all sides, try to think laterally not uni-laterally and if I can’t read your code, then you can’t read and understand it either.
October 24th, 2009 - 20:50
I can see how one would think it is hostile. Though to go over how to write code better would require a lot more time and I do plan on going over how to write good code in future articles.
You did allude to it, but generally well written code should not have deeply nested loop/if structures. Also with well formatted code it shouldn’t be too difficult to find what loop or if you are currently in. As I said in my later article,
these signs are guide lines, not rules written in stone.
October 25th, 2009 - 15:57
I agree with everything, except I’ll pile on with the comments discussion. I agree in principal comments are not necessary in the vast majority of cases. They are necessary when you’re actually coding something difficult that’s not obvious.
You say “I’ve picked up great code written by other developers that had no comments, and I easily understood exactly how it worked.” That’s exactly right – because most code out there is trivial. I’d challenge you to do that the same with some of my NAT traversal code or multi-source downloading code. I don’t know you, but unless you’re on Jonathon Rosenberg’s team at Cisco or something similar, you’re not going to understand it no matter how clean the code is. Read through 200 pages of RFCs plus the plentiful comments, then you’ve got a shot. Otherwise, you’re just not going to get it. If you think there’s a place my code can be cleaner, I’d love to know, but the sheer complexity of the problem is the issue, not the cleanliness of the code.
This has to do with domains and 80/20 type stuff. The vast majority of code you’ll come across is code most people are familiar with, and comments are pointless. Then there’s that 20% that’s way outside your domain and most people’s domains. That’s where you’re screwed without comments.