How clean is your code?
A big part of programming anything is that its readable to the next person who comes along and reads it. Remember that code is read far more times than its written.
Lets take a fairly well known problem as an example. The FizzBuzz problem :
Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz".
A big part of programming anything is that its readable to the next person who comes along and reads it. Remember that code is read far more times than its written.
Lets take a fairly well known problem as an example. The FizzBuzz problem :
Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz".
And a basic implementation of the solution :
public class FizzBangDirty {
public static void main(String[] args) {
for(int i=1; i< 101; i++) {
System.out.print(i + " ");
if(i % 3 == 0){
System.out.print("Fizz");
}
if(i % 5 == 0){
System.out.print("Buzz");
}
System.out.print("\n");
}
}
}
The solution in it self doesn't read particularly well.
- The main method is 9 lines long, arguably too long
- At no point does the method state what its doing, only how it's doing it.
How about a cleaner solution :
public class FizzBangDirty {
private static final int START_VALUE = 1;
private static final int END_VALUE = 100;
public static void main(String[] args) {
for(int i=START_VALUE; i<= END_VALUE; i++) {
printCurrentValue(i);
printFizzBang(i);
}
}
private static void printFizzBang(int i) {
printFizz(i);
printBang(i);
printNewLine();
}
private static void printBang(int i) {
if(multipleOfFive(i)){
System.out.print("Buzz");
}
}
private static void printFizz(int i) {
if(multipleOfThree(i)){
System.out.print("Fizz");
}
}
private static void printCurrentValue(int i) {
System.out.print(i + " ");
}
private static void printNewLine() {
System.out.print("\n");
}
private static boolean multipleOfThree(int i) {
return i % 3 == 0;
}
private static boolean multipleOfFive(int i) {
return i % 5 == 0;
}
}
The cleaner solution has a few downsides :
- Its twice as long.
- Throws reuse out the window.
What it lacks in brevity, its makes up for in explicitly explaining what its intention is for each line of code, not just what its trying to accomplish.
Noteworthy points :
- The for loop explicitly states what is being done on each value in the loop
- The conditions in every if() are a method call, it allows the line to read like an English sentence, giving it far more meaning. So 'if(multipleOfFive(i))' reads simply as 'if multiple by 5' . It also allows the code to be written in the language that descrbed it in the first place.
- No method is longer than 3 lines long.
- There is only 1 public method. Implying that there is only 1 important part of this solution. The rest are just supporting, and can be ignored until necessary to pay closer attention to them.
2 comments:
There seems to be an error in your code, the spec says "print "Fizz" instead of the number "... but your code always prints the number regardless. Anywho, I've had a go myself using a builder to hopefully be more readable and get around the code reuse issue. Its here http://pastebin.com/FaS7G3tX
While it may be tidier I'd argue printFizzBang(i); hides a lot of logic.
The method suggests it would print both Fizz and Bang for every iteration of the loop.
For the sake of readability it might make sense to break it out into
if(multipleOfThree(i)){
printFizz(i);
}
if(multipleOfFive(i)){
printBang(i);
}
printNewLine();
Also, given that the original loop is from 1 to 101 and the tidied loop is from 1 to 100 ypu will get different outputs for the two programs. That's more of an endorsement of TDD before refactoring code if anything though, which I'm sure you'll cover soon.
Best regards
Post a Comment