This is intended to be a non judgmental, collaborative tool to help us all code better, learn from mistakes, learn from others, and an overall resources for devs / design to work together to make more happen. This may also help the team identify strengths and weaknesses within so we can improve our workflow.Just look at how happy these people are after implementing a code review process:
While no exact goals have been set, it might be a good idea to set measurable goals: Have 100% of team reviewing code. Have all code meeting 60-80% of (internal) code standards.
Good ground rules:
- Good, better, best. "Don't stop till your good is better, and your better is best" Break good code, and make it better.
- Be grateful for the reviewer's suggestions. ("Good call. I'll make that change.")
- Avoid innuendo, personal attacks, brutality in review. The code is under a microscope, not the dev.
- Don't be a know it all smarty pants. We are all smart, offer alternative implementations, but assume the author already considered them.
- Accept that many programming decisions are opinions. Discuss tradeoffs,preferences, reach a resolution quickly. Just because this isn't how you would have done it, does NOT mean that it is incorrect.
- Important to remember:
Code should be...
-Functional first,
-Clean and maintainable second,
-Optimized third.
Code should ultimately achieve all three, but the order is important. If the code doesn't work, don't worry about style yet. Similarly, if the code is broken or poorly styled, optimization is only going to make things worse.
- Fork peer's code from github (or send manually if necessary)
- Follow this google form to go through the code. Good tips: Try not to read through more than 500 lines of code per hour, and try to make sure that you thoroughly review the code. A good code review typically takes at least 45m but not more than 90m. form is here
- Branch in github to apply suggested fixes, use a markdown file to make code notes. either way, make sure the dev author knows WHY you are suggesting changes, and what those changes will fix. Including docs supporting your rationale is a good idea. Following up in code review meetings, the dev can explain why they did or did not apply the fix.
For deeper reading into these methods, check the resources section, there are a lot of great links there.
-
Code format and naming. Does the naming of variables follow common sense logic? Does CSS follow a BEM standard? Does the variable or class name state intent (either in comments or in the name itself?) Follow this rule: Do not write comments for what you are doing, instead write comments on why you are doing, the code itself should be self explanatory, but comments/docs help. Examples of statement of intent can include:
"main--slider--wrapper"
or "$vansredbtn"
//
/* this (function, class, mixin) does x */
Follow up: Does the code actually do what the statement of intent declares? Format: There shouldn't be a need to scroll horizontally to find (unminified) code. Make sure tabs/spaces are used where appropriate, and used correctly in markdown/pug/yaml/ts files where they matter. DRY (Do not Repeat Yourself) principle: The same code should not be repeated more than twice. Does this code repeat? -
Functionality Does this code have a readme.md? Can this code be broken? If it can, how? Are there systems or scenarios in which this code would not work? What could be added or refactored? Does this code work in the real world? Does this work across all browsers and platforms?
-
Usability and Interface For any page inspection, try to think of different "scenarios" this code would be useful, or not useful. (From the perspective of a customer that has never used this page, is the design usable? Is it intuitive? Can it be learned quickly?... From the perspective of a shareholder or your boss, does this code drive/encourage the customer to point of sale?) This is important, since the code will be shown to many different people, and we want to make sure it is useful to most. The ten usability heurtistics is an excellent resource(see resources) to check out for user interface design.
-
Security and Vulnerability This one is fairly self explanatory. This is not an all inclusive list, but a secondary security check to ensure code doesn't expose any easy-to-fix security flaws. Is everything up-to-date that can be within npm/package modules? Can any code be injected/inserted at runtime in the application built? Every single image and piece of media being served over secure connection? Is this vulnerable to SQL injections or XSS attacks? (See the cheatsheets below)
-
Code styling,layout,scalability/reusability Explain why the code exists. ("It's like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?"). Are JS functions scoped, is CSS coherent, are similar values group with enum and consts/config/variables used? Run final linters/automated code tests to find any semantic errors, address them. If there is a reason a standard should NOT be followed, then include in the readme.md why that is the case. Could this code be reused in the future? Could this code be scaled (meaning, more users/server load?). Is this code modern (e.g. using new technologies, new methodologies)? Do you and author know why this methodology might be better than another?)
-
Maintenance and future-proofing 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. See "liskov substitutability"
Are you a boss? Do you have a boss? This is why code reviews rock.
The case study (bad picture i know) highlights why peer/code reviews are very effective. A study by smartbear (hp) found that these reviews can HALF the price of bugs found for the company. In addition, even at a cost of $200-1000 per fix,the overall cost is still cut in half.
BEM
OOCSS
CSI Commenting Showing Intent Standard
The Ten Usability heuristics
Liskov substitutability principle
XSS / cross site scripting attacks cheat sheet
SQL Injection hacks