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: The View shouldnt handle Model Logic)

Step 6: Feedback

Useful Link:

9 Code Smells of Preprocessor Use

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, dont commit shit
• if you find a problem/bug you are responsible for it

Good signs:

• Lines of Code
  • The logic/model .m-Files has less than 250 lines of code, (inlcuding comments)
  • The view .m-Files has less than 450 lines of code, (inlcuding comments)
  • The AppDelegate has less than 150 lines of code, (inlcuding comments; please dont rape the AppDelegate)
  • use: find . -name "*.m" -not -path "./" | xargs wc -l | sort to count the number of lines
• Own custom classes for Datasources, (+ there are block based)
• There are unit test
• There is a Jenkins/Xcode Bot for CI
• The harddisk file structur is the same as the Xcode Group structur
• There is less than one 1 singelton for every 1000 lines of code
• The Dev contributes to Stackoverflow
• There are plist's to configure e.g. menues, color, animations etc..
• You can checkout&run the project without any fancy setup
• There is C++ code to share code between Android and iOS