The Checklist of my code review

Definition of Code Review:

According to Wikipedia :

Code review is systematic examination (sometimes referred to as peer review) of computer source code. It is intended to find mistakes overlooked in the initial development phase, improving the overall quality of software.

Ok, I wanna do it! Wait, but …

How to do it?

Reviews can be done in various forms such as pair programming, informal walkthroughs, and formal inspections. The most known is probably this one — show me your code (aka informal review)! Simply ask your peer to have a look at your code.

Git flow

When developing different parts of an application at work we use Git and Git Flow to merge all changes into a parent branch. Once a feature of an app is finished we create a pull request that contains changes to be added to a predecessor branch (usually develop). The best picture showing the flow comes, in my opinion, from GitHub.

  • create a branch from e.g. develop
  • apply your changes to the source code
  • create a pull request to be merged into e.g. develop
  • discuss changes with your peers, explain your point of view & apply suggested improvements
  • your peers approves your changes
  • merge your code into source branch

What should you care about when doing a review?

You definitely should check code integrity — does the style match previous solutions, does it follow agreed conventions? Are features implemented correctly(for this point i would advice if the PR creator could add .GIF image that explain how the feature works that would be nice), does the old source code work correctly after changes?

Why you should care about code review in your development process?

For sure because it ensures code integrity :) , catches what others’ eyes didn’t see. It allows to learn and share your knowledge and expertise, strengthens communication in the team and builds good relationships due to conversations about what you have in common — code and programming skills ;)!

What if you didn’t perform code reviews

Imagine a company X. It delivers mobile app solutions for multiple clients. Their team is small and at some point they cannot meet clients’ demands. So they decide to outsource some of the work to an external company. They give project requirements to this company and meet again after 3 months to receive app source code. But the app is useless — it freezes at network calls, CoreData.ConcurrencyDebug flag crashes all the time. The project is delayed for a few months, team has to start from a scratch. They wish they had reviewed outsourced code on a daily basis…

Do you want to perform code reviews?

This is usually the question that gets lack of enthusiasm from the audience :(. But really, are we too busy to improve??

General Tips:

What i look for during a review?

1. Style

Common red flags 🚩

1. DRY (Don’t repeat yourself)

There are lots of examples for DRY principle let’s take the most simple one:

// let's assume we have a boolean and it's initialised with false
let isPositionCorrect = false
if isPositionCorrect {
updateUI("Position Correct", isPositionCorrect)
} else {
updateUI("Position InCorrect", isPositionCorrect)
}
let message = isPositionCorrect ? "Position Correct" : "Position InCorrect"updateUI(message, isPositionCorrect)

2. Use early exit

The more loops you have the more effort required from our mind to make sense of it. So with early exit instead of having your code wrapped in one if loop statement, you can just add the condition for which the loop is not executed first and just return if that is the case.

func function(items: [Int]) {
if items.count > 0 {
for item in items {
// do something
}
}
}
func function(items: [Int]) {
if items.isEmpty {
return
}
for item in items {
// do something
}
}
func function(items: [Int]) {
guard !items.isEmpty else { return }
for item in items {
// do something
}
}

3. Returning booleans:

func isItemExist(x: Int, items: [Int]) -> Bool {
if items.contains(x) {
return true
}
else {
return false
}
}
func isItemExist(x: Int, items: [Int]) -> Bool {
return items.contains(x)
}
func showImageViewIfEmptyItems(imageView: UIImageView, items: [Int]) {
if items.isEmpty {
imageView.isHidden = true
}
else {
imageView.isHidden = false
}
}
func showImageViewIfEmptyItems(imageView: UIImageView, items: [Int]) {
imageView.isHidden = items.isEmpty
}

4. Remove unused code

The Best Code is No Code At All

Removing code should be one of your activities as a developer. Always delete unused code also code that is generated by IDE. In code review when you see empty method, unused variable, outdated comments, imports hanging around your code, don’t just leave it, it should be removed.

func foo(String: item) -> String {
let baz = findKeyForItem(item)
// we no longer do this for some good reason
// if (baz == "foobar") {
// return baz
// } else {
// return item.foobar()
// }
return baz
}
func foo(String: item) -> String {
return findKeyForItem(item)
}

5. Write “Shy” code

As mentioned in The Pragmatic Programmer, your code should always be “shy code”. Write shy code that doesn’t interact with too many things. Write shy code that makes objects loosely coupled. Write everything with the smallest scope possible and only increase the scope if you really need to — for example, start with everything private, your properties as well as your methods.

class ReusableUIComponent {

private var image: UIImage

init(image: UIImage) {
self.image = image
}

var componentImage: UIImage {
get {
return image
}

set {
image = newValue
}
}
}

6. Hard-coded values

Always think of making things configurable instead of Hard-coding values.

This one of the trivial activities of reviewing code. Whenever you see Hard-coded values, first ask yourself if there is another way you could remove this static value (String, Float, Int,…) and make it configurable? Then go a head and make it better. If there isn’t a way to get ride of it, store it in a static variable (Normally this will be in a constants class/file you use across the project).

7. Language style guide & coding conventions

Almost every company should have it’s own language style guide and code convention e.g Github’s swift style guide. Always make sure that coding style and team standards are followed by everyone. Because doing things your way can be more fun, but your colleagues might not be used to your coding style and if it’s unusual, it will be harder for the next developer to work on what you have built. Your review should include consistent way of naming variables, code formatting, best practice for your language agreed on in your team.

2. Architecture / Design

  • The code should follow the defined architecture, Whatever the architecture is MVC, MVP, MVVM or VIPER make sure that your peers is following it. Also look out for new patterns that could be useful for your project.
  • Check if committed code is reusable. Consider generic functions and classes.
  • Check if committed code is designed correctly and following the software patterns that your team is using, So if your team is following TDD(Test Driven Development) you should always write tests before production code which is mean your code is designed perfectly. From my experience TDD is saving a lot of time for designing your code, but if TDD is not adding value in designing your code, then don’t do it and write your production code directly.
  • Check if committed code is clean code. it should follow Object oriented analysis and design (OOAD) principles or SOLID principles.

SOLID Principles:

  1. S — Single Responsibility Principle (SRS): Do not place more than one responsibility into a single class or function, refactor into separate classes and functions (Plain Objects).
  2. O — Open Closed Principle: While adding new functionality, existing code should not be modified. New functionality should be written in new classes and functions (extensions).
  3. L — Liskov substitutability principle: The child class should not change the behavior (meaning) of the parent class. The child class can be used as a substitute for a base class.
  4. I — Interface segregation: Do not create lengthy interfaces, instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.
  5. D — Dependency Inversion principle: High level modules should not depend on low level modules but should depend on abstraction (Protocols), Which make Dependency injection easy.

3. Testing

  • Unit Test: Check if unit tests have been written for new features. Do they cover the failure conditions? Are they easy to read? How fragile are they? How big are the tests? Are they slow?

Some Swift🕊 related Code Review

1. Bang or force unwrapping ❗

One of the things that i look for when reviewing Swift code is this exclamation mark ❗always look out for code that uses it.

var foo: Object!
print("\(foo.someString)")

2. autoreleasepool

Do you remember autoreleasepool? if you don’t know what is that here is some explanation. Always i pay attention when reviewing body of loop to check if local autoreleasepool could be used to reduce peak memory footprint.

func useManyImages() {
let filename = pathForResourceInBundle
for _ in 0 ..< 5 {
let image = UIImage(contentsOfFile: filename)
}
}
func useManyImages() {
let filename = pathForResourceInBundle
for _ in 0 ..< 5 {
autoreleasepool {
let image = UIImage(contentsOfFile: filename)
}
}
}

3. Mutable vs immutable property

If object’s properties depends on some values, never ever use var. Use let. Always value more immutable state over var. If your property really have to be mutable and another objects needs to access it like from unit tests, expose it to the public as immutable like that:

private(set) var foo: AnyObject
let bar: AnyObject

4. Remove/deregister observers

Whenever you are reviewing or writing code that registers for some sort of notification or subscribes as an observer for updates, make sure you check that they also unregister/unsubscribe. Please check that, because we all know how difficult to debug “Message sent to deallocated instance”.

5. Retain cycles

Avoid retain cycles when using delegates, make sure that delegates are weak. also when using blocks, make sure you capture self as weak.

class Foo {
let bar = Bar()
bar.closeAsynchronous{ [unowned self]
self.barIsClosed()
}
func barIsClosed(){
println("Bar")
}
}

6. Delegates implemented in extension

Make sure that code is readable and maintainable by ensuring that delegates are implemented in extensions.

class Foo {
}
extension Foo : Delegate {
func delegateMethod (){
}
}

7. Use most generic object references

Always make sure when you are using an object, save it to a variable that has the most generic type.

private var viewController: UIAlertController?

init() {
viewController = UIAlertController()
navigationController?.pushViewController(viewController, animated: true)
}
private var viewController: UIViewController?

init() {
viewController = UIAlertController()
navigationController?.pushViewController(viewController, animated: true)
}

8. Unit Tests

In unit tests use ! forced unwrap more because if it crashes on the unwrap this also means the test fails. As an example see testForcedUnwrap

class Foo() {
func getOptional() -> Type? {
return Type?()
}
}
func testForcedUnwrap(){
let foo = Foo()
let optional = foo.getOptional()! //a crash here means the test failed

XCTAssertNotNil(optional)
}

9. Code Documentation

When a class becomes too large, it is useful to add some kind of delimiter (pragma marks) for the public/private methods, the protocol implementations, public/private variables, anything that will help anyone to understand and work with the code and navigate easier through the class.

Final Thoughts:

Code review is a mindset, not a procedure. Please keep an open mind during code reviews. I think this is something everyone struggles with. Try not to get defensive and don’t take it personal when someone says code you wrote could be better.

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store