Cleaning up my code
I've been reading Clean Code that I listed on my list books to get through. I've read a bunch of code improvement books, but there were a couple chapters that I've gone through that I liked so far.
Writing clear functions
We've all heard that we should write functions that only do one thing. But how many of us really do it? Here is an example of how I used to write code.
public void mouseClicked(MouseEvent event) {
// Let's do something on a primary button double-click
if (event.getButton() == MouseEvent.BUTTON1 && event.getClickCount() == 2) {
// do something
}
Inline comments are evil. If they aren't updated, which can happen frequently, why not write a method that can't lie. What I should be doing is writing methods with clear names that inform developers of my intent.
Inline comments can be out of date, in the wrong place, or just plain wrong. Method names get updated automatically if you refactor your code and if the method name is wrong, the compiler let's you know.
Here is how the code should look:
public void mouseClicked(MouseEvent event) {
if (this.isPrimaryButtonDoubleClick(event)) {
// do something
}
}
public boolean isPrimaryButtonDoubleClick(MouseEvent event){
return event.getButton() == MouseEvent.BUTTON1 && event.getClickCount() == 2;
}
The code is trivial, but when your methods and classes get larger, readability and intent become increasingly more important. The longer developers have to stop and think about what your code is doing, the more it needs to be refactored. Here is what a developer might be thinking when they read my old code example:
"We want to do something when BUTTON1 is pressed and we click twice". "What is MouseEvent.BUTTON1"? *Stop to read some java docs* "So we get the click count twice. Oh thats a double-click!"
You might think that someone should immediately associate the clause to a double-click, but you might have to stop and think if you aren't familiar with the author's style or are just getting into Java. Context switching causes breaks in concentration. Not good.
In the refactored code, the developer can get to mouseClicked() and read once, "We want to do something when the user's primary button is double-clicked". No context switching, no breaks in concentration. Good.
Writing clean comments
The next thing I read that was interesting was method level comments. Here is an example of my old method level comments that I would write to pass Checkstyle and document what my method exposes:
/**
* Returns true if the primary button is double-clicked.
* @param event the event wrapping the mouse state information
* @return true if the primary button is clicked, false if not.
*/
public void isPrimaryButtonDoubleClick(MouseEvent event){
return event.getButton() == MouseEvent.BUTTON1 && event.getClickCount() == 2;
}
There are three areas of duplication. First, my initial comment duplicates what the method name states. Second, the @return tag says the same thing. Finally, the method signature clearly states what my method does and returns. Since checkstyle requires a @return tag, here is my refactored method comments:
/**
* @param event the event wrapping the mouse state information.
* @return true if the primary button is clicked, false if not.
*/
public boolean isPrimaryButtonDoubleClick(MouseEvent event){
return event.getButton() == MouseEvent.BUTTON1 && event.getClickCount() == 2;
}
I removed the initial comment to reduce the comment duplication. If the method had no parameters, the code will be reduced further:
/** @return true if the primary button is clicked, false if not. */
public boolean isPrimaryButtonDoubleClick(){
return this.event.getButton() == MouseEvent.BUTTON1 && this.event.getClickCount() == 2;
}
It passes checkstyle and reduces the amount of comment duplication. I like it.
