7 tips on writing clean code
My job has required me to read quite a bit of code written by other people, and over the years I've found that there are some problems that tend to recur again and again. The idea with this blog entry is to write up some tips on this once and for all, in the hope that it can help people out there who struggle to write clean code.
The main goal with these tips is readability. This is important because usually your code has to be changed or reviewed by other people, and writing readably makes these people's jobs a lot easier. That may be obvious. What's less obvious is that it also makes your own job easier, because working on simple code is a lot easier than working on complex code, even while you are writing it, and the more readable code is always simpler.
An important part of making your code readable is to always work to communicate your intent. The basic idea is that it's usually not difficult to see what a single line of code does. The tricky thing is to see why it's doing what it does. So the more you can make your code communicate the thinking behind the code, the better it is. You'll see this point recurring repeatedly below.
I should add that most people seem to be aware of most of these rules, in a general kind of way. The main point of this posting is really to convince people that these rules are important. So important, in fact, that you should try to always follow them, and that you should have a really bad conscience about any code that breaks them. So bad that you just never write any such code. Some of these rules require you to write a little extra code, but I think I can claim that none of them cause you to actually lose time, on any project.
1. Always know why you are catching an exception
This has to be the most common mistake I see people make, and it seems to me that exceptions is a feature many people haven't really learned how to use yet. One of the reasons for this is probably that before the advent of Java the languages people used at university did not have exceptions, and so no rules were taught about this. It's not a trivial feature to use, so it's hardly surprising that people struggle.
Anyway, the cardinal rule is: if you catch an exception you need to have a good reason. Reporting an expected error condition to the user in a more friendly way is a good reason. Handling an expected, but unusual, condition is another. But very often people just swallow the exceptions with an empty catch block (this is a real nono), or they do like this:
private static List readLines(String fileName) {
String line;
ArrayList file = new ArrayList();
try {
BufferedReader in = new BufferedReader(new FileReader(fileName));
while ((line = in.readLine()) != null)
file.add(line);
in.close();
} catch (Exception e){
System.out.println(e);
return null;
}
return file;
}
The catch block here does nothing useful. In fact, it hides useful information if the program were to crash. If you don't catch the exception, you get a traceback. If you do catch it you get much less. Plus, it takes up 5 lines, and it makes the method almost twice as big as it needs to be. Doing without is just a lot better.
That would give us this:
private static ArrayList readLines(String fileName) throws IOException {
String line;
ArrayList file = new ArrayList();
BufferedReader in = new BufferedReader(new FileReader(fileName));
while ((line = in.readLine()) != null)
file.add(line);
in.close();
return file;
}
2. Never declare implementation types
This is another thing I see very often: parameters, variables, or even return types that are really implementation types. You can see this in the code above, where ArrayList is used where List would do. There are several problems with this: ArrayList is longer, and you have to do a lot of work if you want to change what list implementation you are using.
The main thing, however, is that just using List communicates your intent that this be a list much more clearly. It says "I thought about this, and I really intend this collection to be a List, and not just any sort of collection". In other words: it tells the reader that you care about the order in this collection.
Another round of simplification brings us this:
private static List readLines(String fileName) throws IOException {
String line;
List file = new ArrayList();
BufferedReader in = new BufferedReader(new FileReader(fileName));
while ((line = in.readLine()) != null)
file.add(line);
in.close();
return file;
}
3. Use descriptive variable names
This is a rule that can be hard to follow, but in general variables should be named so that they make it clear what a variable contains. Sometimes this is blindingly obvious, and in these cases short names like i for loop indexes, in for files is perfectly OK. But if you use a longer name, try to make it meaningful.
In the case of the method above, what does file really contain? Yes, it does contain the file, but what it really is is a list of all the lines in the file. So why not call it lines? That would give us:
private static List readLines(String fileName) throws IOException {
String line;
List lines = new ArrayList();
BufferedReader in = new BufferedReader(new FileReader(fileName));
while ((line = in.readLine()) != null)
lines.add(line);
in.close();
return lines;
}
Still not perfect, but arguably a lot more readable than the original.
4. Cut-and-paste code
One simple approach that's suggested quite often as a way to improve people's code is to disable the paste function on every developer machine. While the idea may be a little over the top, the basic thought is sound. If you have a snippet of, say, 7 lines of code that do one thing, and you want to do it again to a different set of variables, don't copy and paste the code. Instead, make a function (or a method, if you're stuck with Java).
There are several reasons for this. One is simple brevity. Another is that it makes your code much easier to read, since you would effectively be replacing 14 lines of code with just 2. This would obviously be at the expense of an additional function/method, but since these stand alone and isolated from the rest of the code, their impact on readability is very low. The main reason, however, is that by doing this you make life much easier for yourself. Even if this is a one-time script, you are still going to change the code around. This is easier if it's broken up into parts, like functions/methods.
5. Variables and communication
One of the hardest things to pick up when reading code is to understand the flow of values through the variables in the code. (This is one reason why functional programming is popular.) This has several implications for how the code should be written.
For one thing, this means that you should try to declare variables as close to where they are used as possible. Let's say you write the following:
String tmp = null; // 50 lines that don't touch tmp tmp = whatever; // ...
This means the reader has to scan 50 lines to see if tmp is used anywhere in there. Merging the assignment and the declaration would remove this problem completely, and would lose you nothing.
Another thing this means is that if a variable is only used inside a block (if statement, while loop, or whatever), then it really should be declared in there. Imagine you write this:
int tmp;
for (...) {
// code that uses tmp
}
// no more mentions of tmp below this point
In this case, you could have declared tmp inside the for loop. Not doing it sends the signal that "I'm going to keep using this variable" and forces the reader to try to figure out what happens with it later. Moving the declaration inside the loop means that you can't use the variable after the loop, which makes the code much easier to read.
A third consequence is that you should really work hard to keep your functions/methods short. If you have a function/method that's, say, 400 lines long it becomes almost impossible to track the data flow through it. There will just be too many variables and too many assignments to keep track of. So this is a real no-no. Just don't do it. And as usual with these rules, this is a rule that will also help you get the code right the first time, so this is worth doing even if you will just write the code and then discard it.
In fact, if you really do keep your methods short, a lot of the other rules here become less important, because the impact of breaking them will be lessened. The reason is of course that it's very hard for a short method to be really difficult to read. There's so little code in a short method that you can almost always digest it quite easily. So you could think of this as being the most important rule.
6. Don't preserve return values you don't use
This is also related to making it easier to track data flow. Let's say you write something like this:
boolean present = myCollection.remove(object);
You could have written it like this, however:
myCollection.remove(object);
You can safely assume that any reader of your code will know this, and therefore using the first form is a signal that "I care about this return value, and I'm going to use it for something". If you don't, you'll be confusing the reader, because they are assuming that they now need to figure out what you're going to do with this variable.
Of course, leaving it out saves space, too, so there's really no reason not to drop these variables.
7. Omit needless code!
It's quite common to find code that's commented out, but still hanging around. This is mainly bad because it bloats the code unnecessarily. People seem to do this because they want to have the possibility to bring the code back, either because they are writing an alternative they are not sure about, or (and this seems quite common) because they don't dare to delete it. However, in nearly all cases there is no real reason to keep such code around. You should be using version control, and that means you could always find any deleted code again.
Also, since this code is no longer compiled or executed there is no real difference between commenting it out or deleting it. You've still made the change. The difference is that now it's quite likely that this code will slowly move out of sync with the code around it, so that by the time you find you want it again it will no longer work. That actually makes the code dangerous, because it tempts you to include code without understanding what it does. (If you really understood it you could retype it quite quickly even if it was deleted.)
Of course, commenting out code while you are working on the code is fine. I do that a lot. However, I never check in code that is commented out, and whenever I find such code I delete it, without asking anyone or telling them. If they wanted the code they shouldn't have commented it out. And it will be in the version history, anyway.
Similar posts
Finally solving the performance problem
I wrote about the performance problems the tmphoto application had suffered from, and my failed attempts to fix them
Read | 2008-07-06 14:49
Playing with the Google Maps API
I sat down to try the Google Maps API yesterday, and came away very impressed by both it and the new AJAX style of web applications
Read | 2005-12-03 13:38
A sudoku solver in Python
My girlfriend likes to solve the sudoku puzzles in the newspaper, but I never bothered with it myself, thinking that I shouldn't spend time on something a computer can do for me
Read | 2008-09-10 16:40
Comments
Ricky Clarkson - 2007-05-10 18:44:02
I agree with everything except number 2. It's very rare that I agree with so much, so take that as a compliment.
If you don't want to be able to see ArrayList, then you shouldn't refer to ArrayList at all. If you do List l=new ArrayList() then you're using the static type system to prevent yourself from accessing any extra methods that ArrayList has over List. There's no reason to do that.
If you decide later to use LinkedList (when exactly did anyone last do that) or Vector (hoho!), then you'll get compiler errors, and more importantly, they'll be localised, because you made List the return type in the first place.
The actual mistake is not by client developers, but in the JDK. ArrayList, Vector and LinkedList should not be named types (or at least, those types shouldn't be publically visible). You should just get at them via static methods that return a List.
Clucky - 2007-05-11 06:52:47
Hey, nice post.
But I don't understand #2. That's probably due to my lack of knowledge of Java. I know just about every other language in the world, though. Can you explain #2 in a way that isn't Java-centric?
Svetlio - 2007-05-11 07:21:47
I strongly disagree to #4. Cut and paste is the most powerful development methodology :).
MikeH - 2007-05-11 09:03:44
I saw someone mention something like #2 before and at first I didn't think that it really mattered. Yet I tried it and I decided that it really is the way to go.
If you declare the the return type as an ArrayList, you are saying "This really needs to be in an ArrayList -- nothing else." But the maybe someone wants to use your class and they implemented their own class derived from List. Or maybe it's old code that still uses Vector (I have tons of code that does that). Then they have a problem -- you return an ArrayList and they need to convert it to a TheirList. However, if you simply return the type of "List" they will probably just need to do:
TheirList newList = new TheirList(getList());
Does that help, Clucky?
Rahim - 2007-05-11 09:04:21
Clucky - I'll attempt to explain this, apologies if I cover the obvious!
List is actually an interface in java, ie it provides a contract that an implementation must fulfil.
LinkedList and ArrayList are two different concrete implementations of the List interface, ie they provide all the methods that the List interface says any List should have.
However, in java (and all the object oriented languages I've come across) implementing an interface doesn't stop you including additional methods in your implementation, or indeed implementing other interfaces too.
By declaring your variable as type List the compiler won't allow you to access any of methods of the specific implementation chosen that aren't common to all Lists.
Mike - 2007-05-11 09:22:11
I agree with the post entirely I disagree with all of the comments up to here.
Nice writeup. You obviously have a lot of experience. Thanks for sharing.
Bharat Kondeti - 2007-05-11 09:32:30
I do agree with all the above and would like to add couple of things.
Writing Comments: I definitely believe this comes as part of writing clean code. Comments should be as much descriptive as possible. It does not hurt to write the reason for writing the code the way it is written. The comment should have a distinctive feature like if you see it, you know it's a comment.
private static List readLines(String fileName) throws IOException {
String line;
List lines = new ArrayList();
// Created a buffered reader
BufferedReader in = new BufferedReader(new FileReader(fileName));
// Read each line
while ((line = in.readLine()) != null)
lines.add(line);
in.close();
return lines;
}
private static List readLines(String fileName) throws IOException {
String line;
List lines = new ArrayList();
/*----------------------------------------------------+
| Created a buffered reader |
+----------------------------------------------------*/
BufferedReader in = new BufferedReader(new FileReader(fileName));
/*----------------------------------------------------+
| Read each line |
+----------------------------------------------------*/
while ((line = in.readLine()) != null)
lines.add(line);
in.close();
return lines;
}
Above comments are distinctive..
Providing visual cues in the code: The idea is by looking at the visual cues some things should be apparent. For example in a method we can use ‘Class Variables’, ‘Variables that are passed to the method as parameters’ and ‘variables used inside the method’. If we create separate standard for creating different kind of variables it will be very easy to distinguish them with a glance like all Class Variables should start with ‘C’, all parameter variables should start with ‘P’ and all variables declared inside the method should start with ‘T’. Another example is when we are writing nested for and if statements, its good to provide a visual cue when a ‘for’ loop ends or when a ‘if’ condition ends. Like below example.
for() {
if () {
if () {
} /*end if */
} else {
} /*end if */
} /*end for*/
Similarly giving complete and easily understandable variable names.
Luggy - 2007-05-11 10:04:34
With regard to #5 - be careful when declaring the variable inside of the loop. This will cause the vm to create 1 new instance of the variable for each iteration and they will stick around until garbage collection.
Most of the time you'll be OK with this but if you're dealing with larger objects or large numbers of iterations you should declare the variable before the loop and re-use it.
Mike G - 2007-05-11 10:17:01
Ah, a fellow maintenance programmer. I share your pain. Lots of good comments up there, I shall link to them from my blog.
My pet-hate is the unary-not operator.
My blog here: http://progblog.wordpress.com You'll probably find at least one piece of vitriol that you agree with :-)
Dan - 2007-05-11 11:12:00
After an artima discussion on automatic variables (http://www.artima.com/forums/flat.jsp?forum=276&thread=203222 I've been having second thoughts about declaring local variables as their interface type (though I defended it in that thread).
I've read arguments (sorry, can't find web reference) that specifying an exact return type from the method makes sense. That is, return ArrayList or Vector if that's what's indeed declared, because then the caller is made aware of the performance implications. One would have to use the class type for this to work. (Not necessarily sure I agree with this, just pointing it out.)
But I keep thinking about Properties. Yes, it was a HORRIBLE choice to have Properties extend Hastable, but we're in that boat, it's floating on. When I declare Properties p = new Properties(), I'm saying something specific: I'm using this object _as a Properties object_. (It would be quite odd to say Map m = new Properties(), no doubt.)
When I say List things = new XxxList(), I'm saying that the concrete type is uninteresting: something has to be there, but only for the compiler (I find intriguing Ricky's suggestion of a list factory). When things is declared as XxxList directly, I'm not differentiating between the two cases (where the concrete type is interesting and where it isn't).
If I declare it as the concrete type, when I'm known to use the interface type whenever possible, I'm indicating to the reader that I expect to use methods specific to the concrete type for a specific reason (performance, simplicity, whatever; I always write a comment explaining why when I do something like this). I _expect_ to use setProperty or getProperty, I _expect_ to use removeFirst or addLast.
With an auto-type keyword, I can probably buy that typing that many fewer keystrokes would justify the loss of information. Since we don't have it, however, I prefer to go with the technique that provides more information to the reader.
JollyGoodFellow - 2007-05-11 11:17:49
How about some tips on writing code which doesn't contain a blatant file resource leak?
dudeNumber4 - 2007-05-11 12:34:41
Strongly agree with #5) "declare variables as close to where they are used as possible" and "if a variable is only used inside a block (if statement, while loop, or whatever), then it really should be declared in there". The language I'm using (Delphi - my language of hate), prevents this.
Speedo Joe - 2007-05-11 13:03:45
I couldn't agree with #7 more. It drives me nuts when people keep reams of commented out code in a source file. News flash: There are 100 copies of this in various branches in CVS, we have it if you decide you need it.
Pieter Breed - 2007-05-11 14:46:20
Hey Dude,
Yeah, I really like your post, and the points you raise. I wish more people will take these kinds of style points seriously.
I think it might be interesting/useful to refer to a blogpost that I wrote a few months back, that covers issues similar to these. (I used C# instead of Java though)
http://pieterbreed.blogspot.com/2006/10/c-coding-style.html
Anonymous - 2007-05-11 15:05:16
I disagree with the first part of #5. The old C style of declaring all variables at the top of a block is a great aid to readability, because it gives you only one place to look for declarations. Lets say your code looks like this:
// 50 lines MyClass c = get_some_value (); // 50 more lines c.doSomething (); // what type is c again?
In this case, it is often much easier to have a single block of declarations, so you can look in it and say "Oh, 'c' is of type MyClass".
It also has the advantage of warning you when your function gets too long. More than 3 or 4 lines of variable declarations is a strong indication that your function should be split up.
Tom - 2007-05-11 16:29:35
Why should implementing an interface stop you implementing other methods? What if you need to implement two interfaces? The extra methods may only apply to the specific implementation, and not to the general List. If you only need the methods used in List, then it is best to use List. If, however, you need the functionality that is only available in ArrayList, use ArrayList. If you are given a List, but still need an ArrayList, you can cast it.
I agree with all the comments in here, and wish the people who wrote the code I have to support had read.
Adam Blinkinsop - 2007-05-11 17:28:37
@Ricky: "If you decide later to use LinkedList (when exactly did anyone last do that) or Vector (hoho!), then you'll get compiler errors, and more importantly, they'll be localised, because you made List the return type in the first place."
Actually, you won't. As Rahim explained above, List is the interface that you're using. Both LinkedList and Vector implement it. You're using the List type as a type of documentation: "See, I've got a bunch of data, and it's ordered." If you didn't care about the order, you would return a Collection, and so on.
It's all about making code readable. When scanning a piece of code, the method and variable signatures pop out. Referring to the comments is for understanding why the code was written that way. The best piece of code should be understandable from the signature alone. (For example, "private static List readLines(String fileName) throws IOException" tells me that this opens a file and returns a list of the lines within, even without looking at the implementation. If that's not what it does, it's not very clean code.)
Sarmad - 2007-05-12 03:40:50
I do not disagree with the points made. But there are 2 points I would like to add too.
For point 2 sometimes its better to take the type of derived class you would like to get back. Note you will still return a list. This is useful since it allows you to optimize for different situations with different data structures and allows for easy integration with some old code. Please note that the user can still have the default method which uses the orignal implementation.
For point 4 when at work the supervisors and managers often do not provide sufficient time(nothing against them its just part of life cannot plan everything). Furthermore asking for developing something which is significantly more complicated then any existing stuff. As a result you end up in situations where you do not have time to refactor. Like a simple 10 line method can have many small tweaks to it. As a result you end up committing this sin. But what the bigger sin is that using this excuse to refactor stuff and breaking existing code. I believe if you test the refactor method only then is this point valid. Do not make your code break code which worked. Correctness always comes first(my opinion).
For point 7 it sometimes is good to just make an enum, move the code to it, and use the enum method. This way you have a better chance of maintaining it. Provided you are sure this will be needed at times.
Bob - 2007-05-12 07:15:26
You're not closing your BufferedReaders correctly.
Jonathan Aquino - 2007-05-13 02:55:18
Kent Beck's "Smalltalk with Style" has great tips like these. One of them if I remember correctly is to keep methods small.
Neil Trodden - 2007-05-13 16:24:39
I just want to say as a fairly inexperienced programmer when it comes to oo languages, I was very pleased that I agreed with every point!
The function part (number 4) is something I have repeated many times to colleagues who seem to have concept that code can be "too good" for the task required so lazy spaghetti code is the norm. I actually think they believe that complex code (even when it is unnecessarily so) is some sort of badge of honour.
I used to do point 5 and never really thought about the declaring things at the top of my code much. That's all changed now, thanks for the article.
Roel - 2007-05-14 08:21:37
Please disregard [Luggy - 2007-05-11 10:04:34] This is NOT true in Java. In fact, declaring the variable outside the loop keeps a reference to the last object assigned inside the loop and prevents it from being collected.
Joris - 2007-05-14 08:32:37
A comment on the comment about comments ;) Some "refactoring" guide I've worked with (at school) is strongly against comments inside functions/methods. It states if you need to document what you are doing, split that part out into a method, which by the name of the method documents what is happening; they also strongly recommend writing clear (though maybe long) function names, that really describe what is happening (C-like atoi and such names aren't a great idea, especially not for you own function names)
Also, ending blocks with /* end for */ etc is only really usefull with a number of blocks nested, which implies that block may need to be split into a seperate function..
Steve - 2007-05-14 19:00:19
So, explain again what happens with that BufferedReader if an IOException is thrown while the file is being read?
It doesn't look like any iteration of your sample ever tries to close the Reader if something bad happens..
Baboon - 2007-05-14 20:06:46
most of these are decent tips for simple, localized code...I think the bigger problem is designing clean interfaces, layering and abstraction...so one can get a clear picture when reading code from top-down...
It would be nice to see you post an add on page talking about a few of these points (although, this topic can be covered in huge books :)
Chad Smith - 2007-05-16 15:03:33
Quote Anonymous:
"disagree with the first part of #5. The old C style of declaring all variables at the top of a block is a great aid to readability, because it gives you only one place to look for declarations. Lets say your code looks like this:" <snip> "so you can look in it and say "Oh, 'c' is of type MyClass".
All modern IDEs worth their salt should be able to tell you that information simply by hovering over the variable with your mouse, or in untyped languages, variable prefixes such as hungarian notation can give this information.
Mike - 2007-05-16 22:13:50
With regard to the comment comment (uggh).
There is no need for most of the comments you describe. The code you are looking at is very self explanatory. There is no need to comment on the fact that you are creating a buffered reader or reading line by line. My two year old could see that. You should reserve comments for the parts of your code where the intention is not clear or it has unintended side effects.
It does hurt to have too many comments, especially trivial ones. They tend to get out of date as the code matures and changes.
prashant_vijaywada - 2007-05-17 06:42:54
Hi,
Very good article and i immediately starting to implement 5th and 6th tips.
Khorne - 2007-05-22 10:17:28
@Bharat Kondeti: Are you nuts?
Comments like that requires you to spend a lot of work on formatting them and EVERY decent editor can simply highlight commented areas.
Typoing blocks like that is ridiculous.
Marc de Graauw - 2007-05-22 10:31:04
I don't write Java, but I always catch all exceptions and pass them to a custom error-reporting function which at the very least writes them to a text file and gives a message to the end user. End users often don't write down error codes, which gives you an excuse to blame the stupid end user, but does not help in tracking down the bug. The text file does. The custom error function is also easily adapted to do something smarter: store all exceptions in a database table, send an email or whatever your heart desires in exceptional circumstances.
Marc
Ludwig - 2007-05-27 19:29:36
I'm having a lousy time supporting code that doesn't contain comments. I think they deserve a place in your pantheon.
Habib - 2007-05-30 02:02:43
Nice article. I am little disagree about #7. Sometimes it is very helpful to comments some needless code, specially refactoring, bug fixing. So i think needless code should not always delete.
Vinayak Kane - 2007-05-31 05:10:09
@Khorne:
I think Bharat Kondeti is correct. In India, it's normal that programmers change jobs frequently. Thus other person starts working on the code written by previous developer. And it's always useful for the new fellow to see such comments so that he understands why this is done!
Josh - 2007-06-01 04:13:14
Bharat Kondeti: I hate code written exactly the way you have.
For example, look at your example: // Created a buffered reader BufferedReader in = new BufferedReader(new FileReader(fileName));
Just by looking at it I can see that it's going to be a buffered reader just because it's named appropriately.
You also talk about visual cues, yes these are important however most text editors provide some sort of highlighting syntax so get used to them instead of large comments. This just bloats your file and takes a lot more time up.
Again with your if () { } /*end if */
You shouldn't need to place a comment to signal the end of the if statement; most times I have problems seeing where it ends is because it's nested in which case I have to pay carefully attention to the indentation. However when I do this it suggests to me that my if statement is to long and could be done as a function (which would be 40+ lines of code).
Christian Hauknes - 2007-07-02 03:13:02
@Bharat Kondeti "Writing Comments: I definitely believe this comes as part of writing clean code"
I could not disagree more. Clean code should be virtually comment-free. Clean code should be readable without comments, using good, descriptive names for variables and methods.
Comments get outdated. Code does not.
Billy - 2008-10-27 14:36:31
About #6 - this sort of implies that you are using a function with side-effects - i.e. it both returns a value and modifies some other state. In my opinion, methods should either return a value (and keep all changes local to the method scope) or perform some action and return nothing. Doing both is a confusing thing...
bow - 2009-03-03 20:13:32
Just a small point, but #4-Cut and Paste is really about not having duplicate code around. There is of course nothing wrong with cut and paste as a starting point.
flambard - 2009-04-14 11:04:10
Thanks for the article. It's good to focus people on the cleanlyness topic now and again. Time for spring cleaning ;-)
Of course being a programmer I have my disagreements, most of them out of experience of maintaining, fixing and debugging code of others and my own.
First and foremost keep your methods small. If it doesn't fit the screen its plain wrong (long case statements excepted). A method/function should do no more than 3 to 5 `things', otherwise refine. (Initialisations, loops, if-then-else statments , try-catch statments count as plumbing not as 'thing') !Remember people still use 80charsx32lines displays!
at #5 declare and initialize all your vars at the beginning of the block/scope they are used. Method/function variables at the beginning of the function-block, loop variables at the beginning of the loop-block (if your language lets you). This way you can allways at a single glance check if they are properly initialized. If you cant see where your variable is used or declared your method/function is too long!
Disagree with #2: If there is no specific reason, do not limit the options of the user of your class/method. ArrayList provides methods List does not, and its usage has for example performance consequences. The user is always free to generalize your datatype to a List, but he can only specialize if he knows the implementation. (Since we where designing by contract of course he doesnt ;-)
at some commenters: do not assume the reader has fancy programming tools: maybe he's the support guy trying to fix a bug using a ssh login at the client site. Maybe he fancies the raw editting power of vi/vim/emacs/whatever over auto code completion. Don't assume...
at Bharat: I agree the java/c way of creating block's isn't the most readable, but its the language. Trying to redefine the language this way throws people off even more. If you can't see what's happening: refine. If the indenting is off, use a pretty printer to format it nicely.
Did I say to keep your methods small? Of course I did, didnt I? So make them even smaller ;-)
history of bingo - 2009-05-14 04:21:16
This is important because usually your code has to be changed or reviewed by other people, and writing read ably makes these people's jobs a lot easier. That may be obvious.The basic idea is that it's usually not difficult to see what a single line of code does. The tricky thing is to see why it's doing what it does. So the more you can make your code communicate the thinking behind the code, the better it is. Awesome. just awesome...i haven't any word to appreciate this post.....Really i am impressed from this post....the person who create this post it was a great human..thanks for shared this with us.i found this informative and interesting blog so i think so its very useful and knowledge able.I would like to thank you for the efforts you have made in writing this article.
Mike - 2009-08-04 17:05:21
Do you seriously mean to tell me that some idiot wrote a "how to write clean code" and didn't include the #1 cardinal rule: initialize all local variables to some default value? No wonder the software industry is such a total mess. Wannabes who have no idea what they are doing.
Thomas - 2009-08-20 08:35:46
Use the tools you're given: R#, StyleCop, FxCop, Agent Smith. They will immediately let you know when you did something stupid. Assume similar tools are built-in in Eclipse already when developing Java.
Isn't BufferedReader disposable? Should be in a using(){} statement if it is C#. Dunno what Java does...
@Mike: there are languages where variables are initialized with default values. It's not C++...
Joe Gaber - 2009-10-21 23:35:17
Just a quick comment about comments. I subscribe to the belief that code should read naturally like a book and the need for comments are therefore minimal. I use comments only when a complex concept isn't obvious even with explicitly named classes, methods and variables. For example,
BufferedReader in = new BufferedReader(new FileReader(fileName));
Without comments you have no clue as to what is being read. There is a specific file containing specific data for a specific business purpose. The name of the BufferReader should express this, such as "customerContactDetails". The comment of stating that you are creating a BufferReader is of no value and doesn't let the next programmer or any code reviewer no easily what data is being read nor how it will be used.
Rupeshit - 2010-03-03 23:06:40
This is really a nice post. I completely agree with all tips.Some people says that copy & paste is powerful development methodologies but they are forget that basic oops funda. Thanks for sharing tips.
Rainer Hilmer - 2010-09-23 09:50:44
I wouldn't take #2 too literally. I think what the author meant by this point is: Think about what types you use for a certain requirement and choose what fits best. It's that simple. :-)
Martin Olsen - 2010-10-20 06:25:57
A well written blog post. I agree with all of your points.
Som of the comments diagree with #2 but I agree. The method intuitively returns a list of some kind. If it is declared to return a specific type of list, the reader will pause vriefly to ponder why. Avoiding these little mental hiccups is an important aspect in writing clean code.
I do agree, however, that the point would have been clearer if the declared type was in the list of arguments. In the example, being overly specific about the return type does not necessarily impact the calling method, since it may just choose to store the return value as a List. But if the calling method chooses to store the return value as an unneccesarily specific ArrayList, we have introduced unnecessary inflexibility ..
If the discussion were about an argument to the method, it is hopefully clear to everyone that requiring an ArrayList is plain silly if requiring a List (or an Iterable) would do. It introduces an unneccessary constraint on the calling method.
Another angle upon this is that programming to interfaces will never come back to bite your heels while programming to implementations will often do so.
The comment which equates good style with writing comments all over the place is very wrong. The "end if" and "end for" comments just reinstate information that should be obvious from the indenting (if the indenting is wrong, ask the IDE to fix it). Commenting small code blocks with obvious and adorned comments pollutes the code. This style of commenting is not only a waste of time and a violation of the DRY principle, it is also hazardous. As every experienced programmer knows, eventually code and comments will drift apart to the point that the comments will be absolutely misleading. So experienced programmers trust the code and resort to the comments only if the code is unreadable. But there is no excuse for unreadable code. My rule of thumb: Use comments to explain the non-obvious, the catches, the tricky algorithms. And use them to document interface classes as they evolve into stable APIs.
Jonathan Wood - 2011-01-03 23:25:48
I've read your #1 tip from several sources before. But I must say that I just can't agree with it.
First of all, I've seen code where you have all these catch blocks but then, often, they all do the same thing anyway. After all, in most cases all that's needed is to notify the user what happened. In this case, the extra catches are just a waste of code.
I've also seen code that caught specific exceptions, but then did nothing for other exceptions. That's no good. And even if you write a catch block for all possible exceptions, what if your code runs on a future version of your platform that introduces new exceptions?
There are occasions where I need special handling for a specific type of error and I'll create a separate catch block to handle that case. But, otherwise, all that's really needed is to report the error with a reasonable message. (In C#, it's something like "Unable to retrieve data from the database : " + ex.Message.
Lars Marius - 2011-01-04 02:24:55
@Jonathan: I don't think you understood the first point. The point is that you should generally avoid catching exceptions at all. They should only be caught when you have a positive reason to do it.
In the example I use, catching the exception is bad practice because the only thing you're doing is to throw away the stack trace. So you're bloating the code to do nothing useful, but removing useful debugging information.
Kenneth - 2011-06-23 04:53:08
I agree - totally! One thing that has been the hardest for me to let go is to comment code... I used to be a frequent commenter of my code, but now when I understand the principle of clean code I've not only have stopped commenting, but also removing comments...
But I really think You've forgotten one really important thing when discussing "clean code" is : Single Responsibility Policy. That says that a function should do one thing and one thing only and doing it well and not doing anything unexpected. Following that policy will automatically give You small functions that are easy to read and understand.
Following the policy will lead You on the path of removing boolean arguments on functions... The true/false junction will almost always automaticlly break the policy by letting the function doing two things; either this or that depending on the boolean value...
Every programmer should read the book "Clean Code: A Handbook of Agile Software Craftsmanship" by Robert C. Martin (http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882
It explains all Your points as well as tons of other good stuff (such as Single Responsibility Policy and Dependency Injection). The book is written for Java developers, but I have found that the "clean code" stuff is applicable on almost every other language.
rick - 2012-03-20 09:08:48
I think #4 is a bit tyrannical. Cut and paste javascript codes is a much needed option for web designers who do not have experience in javascript coding. Yeah, I know, go and learn, but there are many, many javascript coders who write up these free and paid codes for a living. So you are saying that they should quit their profession?
All the other tips are good though...
ling091 - 2012-04-27 22:12:37
#2 presents the interface-oriented programming instead of the implementation-oriented programming, the former provides better abstraction. Interface-oriented programming is the base stone in OOP. Design patters emphasizes a lot about this.
#4 Code clone is really a bad programming habit and I have suffered a lot from finding the implicit bugs inside the code coped from somewhere. Obviously, we copy code because some code does similar task but not really the same, then we need to make some modifications to the code. Right at the moment you probably plant a hidden bomb by forgeting to change some lines. You spend much effot to find it and finally find it is simply caused by your inattention. That makes you depondent!!! Actually there are many papers cares about how to searching the cloned code for refactoring.(Google by "clone code").
Lucifer.iix - 2012-06-25 05:49:48
"Correctness always comes first(my opinion)." (Ohhh, my god ???!)
Don't tell this to your software tester! Because he (or she) isn't quite shure about your correctness.... :-)
Maybe this says everything: Why software cost a lot of money and it looks most of the time like total crap that you don't expect from so called expers. (Expert: You don't have to review my code, Trust me !)
Testability comes before `Correctness`, how else do you know it`s correct?
You know what comes first? Make a good understandible design: So, Turning off your pc and start communicating (brain-storming) with your team !
Lucifer.iix - 2012-06-25 06:23:38
"The code you are looking at is very self explanatory."
Of course it is, it's written in `a` language you can read ! The most stupid thing todo is writing/maintain something in 2 languages.
It's just like saying: The rest of the company doesn't understand our software development language. Let's translate it for them! (With all the bad translation that comes with it!)
No: You better write, "WHY" did i write it like this... And what the f^&k was i thinking back then! And why am i thinking this is a right thing to do and also writing things down that may not or can`t happen in practise where the `thinking-logic` stops. So if you `assume` it only works with positive numbers: "Write it down! (For testing!)". Other wise a good tester will come to you and say: Do you know your code, doesn't work with negative numbers !! And is your software prepared or checking for this event? Is it posible to get negative numbers? And how do you know that (Mr. Expert?)...
So if you don't document well: Your tester gone ask you a lot of questions ! Or: He just will ignore you completely after some weeks!
So you can write down something like this:
** It only works with positive numbers, but nobody wants to get negative money back so we (names go here!) think its `ok` to do....
or ** Result's can be negative if .... and also.... but it's very impratical and (names go here!) decided to go for it like this. for more information: http://mycompany.crap/software/development/version/delopment_meetings_923487#there_you_go
Lucifer.iix - 2012-06-25 07:08:29
@Christian Hauknes and @Bharat Kondeti "Writing Comments: I definitely believe this comes as part of writing clean code"
I could not disagree more. Clean code should be virtually comment-free. ---------------------------------------------------------------
And with a warning sign at the top: Warning made by: `Christian Hauknes` he worked here for a couple a years before we fired him. We have now idee where he is now, but: `good luck with his code!`
Example: // Settings text to a default string: "(default)" string text = "(default)"
THIS IS NOT A COMMENT IT'S A LANGUAGE TRANSLATION !!!
(Wiki: A comment is generally a verbal or written remark often related to an added piece of information, or an observation or statement.)
Comment looks like this: "[-Date-][-Author-]
Normaly we use a constant string as the default text, but in link://whatever/document#whyWeDidntWantToUseOurOwnPatterns there is a section WHY we dont do it this time around. And why our boss thinks its a great thing to do!"
Hope this helps to understand what the difference is between having a comment on a piece of code. Or just writing it twice (with bugs included) in a different language....
Grant - 2013-01-25 17:25:55
With regard to #1, I think it is not necessarily correct. I'm writing code for nodejs as a server.
If I try...catch the whole code, then output error.stack, it will still output the error with the stack trace. But what it can do is that it will stop the server from crashing.
If an exception is not handled by a mass handler, it will cause the server crash and stop server from functioning until someone restarts it.
A mass handler will also allow you to output a little more info if you know other variable values are useful and want to output along with the error stack.
Lars Marius - 2013-01-25 17:29:07
@Grant: I believe you are actually following rule #1. You know why you're catching the exception: to prevent the server from stopping (and perhaps adding some additionao debug info). Sounds fine to me.
Michael@programming - 2013-05-30 04:34:02
This is a pretty straightforward list of dos and don'ts that every programmer should have learned when actually learning programming. Every programmer should know that code shouldn't be redundant or more complicated than it is needed. A serious problem as you stated in the first tip is that some universities tend to teach old programming practices and paradigms.
james s. - 2013-10-28 17:31:29
I like that this discussion is still ongoing. I've always liked what the Webkrauts did with http://coderesponsibly.org - simple but what's important for code.
Gnurf - 2014-06-05 09:42:18
In your example, after "int tmp", you should show an "if" rather than a "for", because one can imagine some cases where you want to share a common variable between all iterations of the for.
p2r - 2015-03-13 04:01:15
Still a very good post, with lots of good comments (and a couple of disastrous).
Here's one more comment on comments vs clean and self explanatory code ...
In terms of code readability and maintainability I think the single most important factor is naming conventions. With good and clear names, comments are redundant. With small, focused classes and methods, crisp naming is easy. As stated before, you need to comment what is not obvious or expected, and minimize this need using good naming conventions and clean code principles.
Tim Ottinger's article on naming conventions is still the best I've found: http://objectmentor.com/resources/articles/naming.htm
Quoting the introduction and final remark in the article: "Source code is a form of expression and so quality-of-expression must be a primary concern. Poor quality-of-expression creates ongoing costs and impacts the productivity (hence the reputation) of the team that produces it. Therefore software authors must learn to clearly express the intention of variables, methods, classes, and modules.
Furthermore, maintenance errors occur when a programmer does not correctly understand the code he must modify. The diligent programmer may expend considerable effort to accurately decipher the true intent of a program, even though its purpose was obvious to the original author. Indeed, it takes much more time to determine how to change the code than to make and test the change."
"The hardest thing about choosing good names is that it requires good descriptive skills and a shared cultural background. This is a teaching issue, rather than a technical, business, or management issue."
Tim - 2015-09-11 09:33:18
Good post
fadirra - 2015-11-29 17:37:06
Nice tips, but even those tips have exceptions!
Bimal - 2018-09-04 03:04:19
How about..."Never Hardcode"?