Review for logic errors
Have they considered nulls in likely cases, does the way it uses and interacts with other parts of the system make sense? If they have used patterns do they make sense or are there any they should have used?
Review for architectural issues
Is the seperation of code done well, think about SOLID and coupling and cohesion. Often at this point its worth just asking them questions about their thoughts on it and what their approach was. Just make sure they have thought about it and their thinking is sound.
Review for over engineering
Have they added things that aren't necessary? We are all guilty of this, and they may not need to remove them as they have already been done, you might keep them if they don't cause too many problems. Just let them know for the future, PR's are as much for training as they are code review.
Review for readability
Readability is how easy it is to read, not if it conforms to your internal version of what is well styled, does the code have confusing knots of code? i.e. the code is too compressed and hard to comprehend? Not are the line breaks in a consistent manner, this one is hard to call, my main suggestion would be try to care less than you think is important about style as it adds very little and there is a difference between style and readability.
No comments:
Post a Comment