Today I did a code review for the first time. I’ve certainly looked at a lot of chunks of code on github, stack overflow, and other blogs before, but today was the first time I’ve actually read through someone else’s code to get an understanding of every part of their application in order to give feedback.
I reviewed a mastermind game which I have also built and have been working in various forms for weeks. I have a very good handle on the logic, which should make it fairly easy to read someone else’s code. But it turns out that wasn’t the case.
This exercise gave me flashbacks to all of the times my mentor has given me feedback. It also made me realize how important it is to write readable code. Here are some important things to remember when writing a program that others will need to read (which you should always assume will happen).
Very Descriptive Method Names
Method names should describe what the method is doing in detail. It can be lengthy as long as every word is necessary and you’re describing what you’re doing. You should be able to tell from the name, every important thing that will happen inside that method. If you can’t, then you should be splitting your methods up into smaller methods.
Here are some examples.
Bad method name: get_input
Better method name: ask_user_for_menu_selection
Bad method name: run
Better method name: play_game
Bad method name: valid?
Better method name: valid_main_menu_selection?
Bad method name: ask_user_for_menu_selection_and_first_name (two things are happening here!)
Better method names: ask_user_for_menu_selection AND ask_user_for_first_name
Use Less Instance Variables
You should only use instance variables if you have to. When I see an instance variable it immediately grabs my attention because I know it’s important enough to be used in other methods throughout the program. If it turns out that the instance variable isn’t used elsewhere than that leaves me confused.
This also ties into dependency injection. Whenever possible, try to inject your variables into your methods instead of using instance variables across methods. This makes them easier to test and easier to remove/separate from the application.
Create Methods for Rules & Business Logic
Let’s use some examples here.
Let’s say a user is only allowed to select 2 or 4, otherwise an error_message is displayed.
if selection != 2 && selection != 4
selection != 2 && selection != 4
The first example leaves you wondering ‘Huh? why do I care about 2 and 4?’ The second code example gives us an understanding of why the 2 and 4 matter.
Display Methods In The Order They Are Called
I don’t think anyone has ever actually told me to do write my methods in the order that they are called, but I find it very helpful when trying to read someone else’s code. It’s much easier to see what’s happening if the methods are in some order than if they are scattered throughout the class. If this is a concern, it’s also probably a sign that your class is too big.
Watch Out for Unnecessary Code
This might seem obvious, but remember to remove any variables or pieces of code you’re not using. I know that this has probably happened to me and I noticed it in my code review. When refactoring, you might eliminate the use of certaing variables and methods. Don’t forget to take them out. It’s very confusing to see a variable iniitalized with a class and then not have it called anywhere.