codereview.md

How to do codereview

Step 1: Context

Check what should have been done

Step 2: Git Changes

Check the related git commit

Step 3: Syntax Check

Check if the Dev use the right syntax and follow the styleguide

//wrong
if (error != nil) { ...
//better 
if (error) { ...

//wrong
CGFloat width = self.frame.size.width;
//better
CGFloat width = CGRectGetWidth(self.frame);

I suggest the NYTimes obj-c styleguide

Step 4: Logic Check

Check if there is duplicated code

//wrong 
view1.backgroundColor = [UIColor blueColor];
view2.backgroundColor = [UIColor blueColor];
view3.backgroundColor = [UIColor blueColor];
//better
UIColor *backgroundColor = [UIColor blueColor];
view1.backgroundColor = backgroundColor;
view2.backgroundColor = backgroundColor;
view3.backgroundColor = backgroundColor;

//wrong
if ([self someBoolValueCheckingWithValue:someValue]) { ...
...
}
...
...
if ([self someBoolValueCheckingWithValue:someValue] && someOtherBoolValue) { ...
...
}

//better
BOOL checkedBoolValue = [self someBoolValueCheckingWithValue:someValue];
if (checkedBoolValue){ ...
...
}
...
...
if (checkedBoolValue && someOtherBoolValue) { ...
...
}

Step 5: Structure Check

Check if the code is placed in the right classes and/or files.

You should ask yourself the following questions:

• Should we maybe move the code into a Category?
• Do we really need this property in the Header File (e.g. has it be public)?
• Can we move the code and logic to a more generic extra class? (-> Can we generalize this code?)
• Is the view code really needed in the controller/model?
• Beware of strong reference cycles. Do we really need this strong property or does a another object 'retain' the needed object (like the superview)?
• Do we really need this property or can we assign the needed value directly in the method call? (extra properties are optimzed out by the compiler and make debugging easier!!!! not yolo, write code for the programmer not the compiler)
• Should the view really request the needed informations (e.g fetchrequest from CoreData) or should we pass the information to the view directly. (Tip: View should handle Model Logic)

Step 6: Feedback

Tipps:

It should be a warning if there are:

• many if/else's
• nested if/else, like if(){ if(){}else{} }
• not localized NSStrings
• duplicated code
• blocks in blocks (shame on me)
• no usage of pods, ARC, blocks
• no constants
• directly use of iVars in non setter/getter methods
• no usage of weak, strong and block references
• are there views that have model logic (e.g. fetching data)
• write subtle comments, why you are doing something, not how
• and if you are solving a (redmine) issue in that fixed some non-obvious edge case, add the issue link as comment, so that the next programmer understands why you had to fix something
• commit often