Good code doesn’t need comments…
It’s a quote from myself, one which startled up some discussion in my team and colleagues. I still stand by it, although it does need some extra explanation:
Good code doesn’t need comments. With comments I mean non-javadoc comments inside a method body. If you need to write comments to make your code readable or understandable, the code is just too hard. It’s a good tip: extract a method if you need to write a comment to explain something. For instance:
public void foo() { //I need to check this because of blabla //the compareTo is to catch the foo //exception that else occurs if (something == somethingElse && foo.equals(bar) && !bar.compareTo(foo) < 0) { ...doSomething} }
I believe strongly it’s far better to extract the if body into a seperate method, and write the documentation inside the javadoc.
something like:
public void foo() {
if (isDoSomethingNeeded()) {
...doSomething
}
}
/**
* Check blabla
* also compareTo so no foo exception occurs
*/
private boolean isDoSomethingNeeded() {
...
}
- The code becomes self documenting
- The comments are reused when other methods start invoking the “though logic”
It’s an absolutely stupid example, but the rule still stands:
Good code doesn’t need comments!
i agree with you and i do it the same way. I also believe that it doesnt need javadoc and javadoc should only be used if you need to explain something or write it to not forget it.
For example:
public static void removeDuplicates(Collection col){…};
I know you can write great javadoc comment for that method and even go crazy with IllegalArgumentException, but should you?
I believe javadoc should be something special and not just a must for every public method. When im in the jungle of javadocs i just ignore them all.
I’d still javadoc the removeDuplicates(Collection coll) with something like:
Removes duplicates from the collection.
Duplicate nulls are removed as well.
@throws IllegalArgumentException in case of null collection
There are not a lot of methods that just do what the methodname reveals. Most of the time more stuff is going on.
On another note, It’s important to only document behavior, not implementation. It would be wrong by revealing the collection is added to a Set to remove the duplicates.
I don’t agree at allwith this statement:
“It’s a good tip: extract a method if you need to write a comment to explain something.”
By doing that, you’re totally misusing methods. You should not forget that a method is also a reusable chunk of code, so after you extract it, it will be available to other people working on the same class at least. And I’ve seen that so many times: people see this private method whose name looks like it does what they are trying to achieve so they use it without wondering and boom! It’s like a bad old copy/paste that people forget to adapt and you have a big bug.
My point: if you have complex code in a method, ask yourself first if it would be useful as a generic reusable method. If yes, extract it. If not, comments are your best option.
isnt that mistake made by people that use the method? if for example you add new const to enum and do nothing except that some code could break. you should look where enum is being used and refactor/correct that code. i saw many multi-paged methods and problem with that is that its very hard to understand them and even harder to try to understand. when i meet very complex method i dont read it, but debug it from end to start and stop when i find where bad data is coming from(they are too long to debug from begining, so i shoot with breakpoints). I think the problem you described is when private method should be in util class.
@sarbogast
The habit of extracting “self-describing” methods come from the book Refactoring, by Martin Fowler.
When reusing private method, and not util methods like raveman stated, is always a risk, since you are reusing an implementation and not a behavior. Private methods are most of the time not designed to be reused.
What I like to do is write the public method, where all I do is invoke private methods. The public method acts as the algoritme, defining what has to be done without any implementation details. The implementation is then handled by the private methods.
This is definitely not only self documenting. By even making the algorithm method protected, or define in an interface, the core behavior can be isolated and overloaded thoroughly as one prozaic decision method, or later even easily teared-off to a rule engine implementation.