Lesson Learned From Code Review

I had rounds of code reviews for my PR to add Route53 DNS module to Starchart. Here's what I learned from the code review.

Working code is not enough, it should be easily readable for other developers

I wrote a createRecord function that creates a record in Route53 hosted zone. At the beginning, it didn't have hostedZoneId as I was thinking we would use only one hosted zone and I could treat it as an environment variable. Then I needed ability to set a hosted zone for testing purpose and added hostedZoneId to the function as the last parameter. But this is not a good practice. Some people would think hostedZoneId is something that exists inside a record. It works same if I add hostedZoneId in any order in the function, but the code should look clearer for others to understand.

Don't let same logic duplicated on the code

Even though I know this rule and try not to write same logic in different functions, but it occurs when I don't look at my working code closely again. Duplicate code can be reduced by reviewing and refactoring multiple times before pushing it to repo. Again working code is not enough. Repeating code will result in bugs once someone fails to modify all of the same logic. Good refactoring skills come after a lot of practices. Once you confirm that the code is working, it's time to find places to improve.

Function should do only one thing whenever possible

This is a good principle to write good tests or write tests more easily. Also it's good to produce the right error message when something breaks. Function name should be as clear as possible. validate() does not give any clue what to validate. If it can be broken down to validateRecord() and validateDomain(), it will be much easier to read and probably people don't have to look into function implementation.

Don't use unnecessary syntax and use linter to set a rule to maintain consistency

Which code looks cleaner and easier to read?

function foo() {
    if (x) {
        return y;
    } else {
        return z;
    }
}

function foo() {
    if (x) {
        return y;
    }
    return z;
}

Both work same, but we don't want to write unnecessary statement. We can avoid unnecessary syntax or error-prone code by using linter. It will help you get used to certain principles.

Conclusion

You will look much more professional if you look through your code and improve before someone reviews. Also thoroughly take reviewer's comments and discuss if you have a different opinion. Opinions are always debatable. You will learn more from discussion.