Do you do code reviews? If so, why? What do you benefit from your code reviews?
Most companies do code reviews to prevent future program errors, but in my opinion a major benefit of code reviews is to ensure the code is maintainable, that others can read and follow the code.
If you have not read the book Clean Code by Bob Martin, I highly recommend it, as it will give you techniques for writing maintainable code.
Code Review Fridays
Here at BlueFletch we have a standing weekly code review meeting. The engineers meet as a group and review some portion of code from a current project. This meeting does not replace individual project code reviews, but is meant to benefit everyone in the company not working on the project.
Some of the benefits are:
1. See what others in the company are working on
2. Get insight into different technologies
3. Get help with a tricky piece of code
4. Ideas on doing a project differently
During our code reviews we look for the following items:
1. Readability — is the code easy to read / follow
2. Maintainability — can our customers easily maintain it
3. Usability — are the classes easy to use, are they overly complicated?
4. Does the code make sense?
Additional items we look for can be found here
A Recent Review
On a recent Friday we reviewed a couple of Android projects we inherited from other companies. As per our practice, we reviewed the code from the aspect of clean code and best practices, discussing how we would make the code maintainable and easier to read.
During the review four major issues jumped out:
1. Pyramid of doom
2. Project organization
3. Code Reuse
4. Hardcode Strings
Pyramid of doom (or Callback Hell)
Every engineer has run into this, regardless of the language or platform, you have a function that has many, many levels of indention. This could a HUGE if statement with many conditions or a series of callbacks within callbacks. The deeper the code, the harder it is to follow and maintain.
Example:
[code]
if ( conditionA) {
if ( conditionB ) {
if ( conditionC ) {
block of code doing something 1
} else {
if ( conditionD ) {
block of code doing something 2
} else {
block of code doing something 3
}
}
} else {
if ( conditionE ) {
block of code doing something 4
} else {
block of code doing something 5
}
}
}
[/code]
Tip: If you find yourself coding such a monstrosity, STOP! Refactor the code. Start with the inner code and place the code into separate methods. Yes, you end up with more methods, but the code will be more readable, and with smaller methods you get code that is easier to write unit tests.
A possible rewrite (one of many ways, but easier to write unit tests):
[code]
..
if ( conditionA) {
methodConditionA()
}
..
methodConditionA() {
if ( conditionB ) {
methodCondtionB()
} else {
methodCondtionE()
}
}
methodCondtionB() {
if ( conditionC ) {
methodHandlerProcssingLogic1 ()
} else {
methodCondtionD()
}
}
methodCondtionD() {
if ( conditionD ) {
methodHandlerProcssingLogic2()
} else {
methodHandlerProcssingLogic3()
}
}
methodCondtionE() {
if ( conditionE ) {
methodHandlerProcssingLogic4()
} else {
methodHandlerProcssingLogic5()
}
}
methodHandlerProcssingLogic1() {
block of code doing something 1
}
methodHandlerProcssingLogic2() {
block of code doing something 2
}
methodHandlerProcssingLogic3() {
block of code doing something 3
}
methodHandleProcssingLogic4() {
block of code doing something 4
}
methodHandlerProcssingLogic5() {
block of code doing something 5
}
[/code]
Project organization
Within one of the projects we reviewed, there were 67 fragments, all within one package. For non-Java programmers, imagine 67 code files within one folder, with the files having similar names.
There were 11 fragments referring to devices; 4 related to signing in; various fragments for product registration, and 2 About fragments.
How are these 67 fragments related? Which ones need to be looked at when enhancing a feature of the application? Where do we start?
Tip: How you organize your code can help those that follow. If you make it easier to identify features and/or the process flow, then those that follow will be able to understand your project.
If the engineer building the project had separated the fragments out by features or relating the code in some manner, then working with the project would be easier.
Some suggestions for organization include placing all the “User” related (sign in, forgot password, account creation) into one package. Place settings related fragments into another package. Place device related fragments into another package.
Get the idea? Organize the project by context.
Code Reuse
Within one of the projects we reviewed the code utilized simple REST API calls to gather data. A simple pattern of URL creation with HttpUrlConnection classes and code to handle the result.
After some research we noticed the same endpoint was used in multiple places. Then we found the same exact code pattern, almost line for line, in 15 other places. The code had been copied from place to place, with minor changes to suit the need.
And there was a bug within the code that required a major change within each piece of code. Yuck!
Another project we reviewed had a common error message display, but the display code was copied into each Activity (25 code files). Imagine if they decided to change the look/feel of the error display, 25 code files would have to be updated and tested. Dooh!
Yes, as software developers we need to get things done fast, but the consequences of copying code from one part of the project to another will cause you headaches down the road. As no code is perfect, at some point it will need to be fixed or enhanced, how many places do you want to make changes?
Tip: if you find yourself copying a block of code from one part of the project to another, almost line for line, then build a reusable code object, either a new method, a new base class, something common. This will reduce the amount of code to maintain, and if a bug does arise, you have one location to fix it.
Hardcode Strings
Imagine opening a code file for the main activity of a project and having to scroll through 1,300 lines of hardcoded string literals? Yes, we had to do that during our code review.
We as engineers get lazy, we need to add a new UI label or a new error message, instead of doing it correctly we put the text directly in code. It never fails, later the project needs to be localized and now you have to pull all the strings out to a string resource file. How time consuming! How much time does it take to add the initial string to the resource file, less than a minute?
Tip: Take the time up front, do it right, and for the sanity of the engineer that follows you into the code base, do not hard code 1,300 lines of string literals into the main activity.
Conclusion
As software developers/engineers, we should take pride in our code. We should build our code to be readable, maintainable, and testable. Doing code reviews either as a group or individually will help you and others to learn and improve code.
And please: organize your code, write reusable modules, do not hard code strings, and break down your large unruly code blocks into smaller methods.
