How to conduct a code review?
At work, they offered to read a report on the topic of development. Voila! Next I will tell you:
What is a code review?
Why is it needed?
What are we checking?
Common Problems & Solutions
BONUS!!! View Poll Results: How do you code review?
Presentation available link.
What is a code review?
Codereview is, all of a sudden, a code review. Developers write the code, then make an MR (merge request), others check it, fix the comments and upload the code to develop. You can, in principle, immediately upload it to develop, but it’s better for someone to look at your work before that.
https://en.wikipedia.org/wiki/Code_review
https://en.wikipedia.org/wiki/Software_review
Why do you need a code review?
Basic things that developers usually mention:
improve code quality (style, understandability, maintainability)
find errors
transfer knowledge
increase mutual responsibility
offer the best solution
check for compliance with guidelines
From the available statistics, I googled the report “The State of Code Review 2017» (link under VPN):
What do we check for code review?
There are a million articles for “code review checklist”: 1 2 3 4.
The main criterion: the code should be understandable to you. “Write code as if it will be supported by violent psychopathwho knows where you live.
First of all, pay attention: small methods with simple code; removal of constants, formatting (line length, names), comments, release of resources (using, unsubscribe), access modifiers, design guidelines.
It’s great if you know the code and can suggest how to improve it. For example, a person wrote a method, but you know similar code in another class and it is better to move the common functionality into a base class or utility.
There are checklists for different languages, for example, link for python.
https://en.wikipedia.org/wiki/Coding_best_practices
https://en.wikipedia.org/wiki/List_of_software_development_philosophies
Common Problems & Solutions
1. Smelly code | 6. SOLID, DRY, KISS, YAGNI, BDUF, APO, Occam’s Razor |
2. Scented design | 7. Vulnerabilities in the code |
3. Refactoring | 8. Code rot |
4. Antipatterns | 9. Dark patterns |
5. Design patterns (templates) | 10. Static code analysis |
Below, we will briefly review each section without detailed comments. See the links at the beginning of the section for more details.
Smell Code
https://en.wikipedia.org/wiki/Code_with_wrap
Code duplication | lazy class |
Long Method | Theoretical generality |
big class | Temporary field |
Long list of parameters | Call chain |
Divergent modifications | Mediator |
Shotgun shooting | Misplaced proximity |
Envy functions | Alternative classes with different interfaces |
Data groups | Library class incompleteness |
Obsession with elementary types | Data classes |
switch statements | Renunciation of an inheritance |
Parallel inheritance hierarchies | Comments |
2. Scented design
https://en.wikipedia.org/wiki/Design_smell
Lack of abstraction | Broken modularization |
Multifaceted abstraction | Lack of modularity |
Duplicate abstraction | circular dependency |
Insufficient Encapsulation | Non-factorized hierarchy |
Unused Encapsulation | Broken hierarchy |
3. Refactoring
https://ru.wikipedia.org/wiki/Refactoring
Change the signature of a method | Parameter introduction |
Field Encapsulation | Lift method |
Class selection | Descent method |
Interface selection | Renaming a Method |
Allocation of a local variable | Move method |
Extraction method | Replacing the conditional operator with polymorphism |
Type generalization | Replacing Inheritance with Delegation |
Embedding | Replacing type code with subclasses |
Factory Introduction |
4. Antipatterns
https://en.wikipedia.org/wiki/Antipattern
development (in OOP, when writing code, methodological, configuration management, miscellaneous) | when reusing code |
architectural | in human-computer interaction |
organizational | service-oriented architecture |
environments | for intelligent information system |
information security |
My favorite anti-patterns:
God object: The concentration of too many functions in one part of the system (class).
Unnecessary Complexity[en] (Accidental complexity): Adding unnecessary complexity to a solution.
Premature optimization: An optimization at the design stage of a segment of code that makes it more complex or corrupted.
The sin of premature optimization. boat anchor[en] (Boat anchor)[13]: Save a part of the system that is no longer used.
Magic numbers: Using numeric constants without explaining their meaning.
Copy and paste programming[13]: Copying (and lightly modifying) existing code instead of creating generic solutions.
Reinventing the wheel/bicycle[13]): Building from scratch instead of using a good off-the-shelf solution.
Big ball of mud: A system with an unrecognizable structure.
Analysis paralysis[13]: unreasonably high costs for analysis and design. Often leads to the closure of the project before its implementation begins.
Creeping featurism: Adding new features at the expense of the overall quality of the system.
Boiled Frog Syndrome – gradual negative changes are noticed when it’s too late.
https://blogs.oracle.com/javamagazine/post/five-code-review-antipatterns
5. Design patterns (templates)
https://en.wikipedia.org/wiki/Software_design_pattern
https://ru.wikipedia.org/wiki/Design_Pattern
Main: | Flexible Object Programming |
Fundamental | Task execution |
Generative * | System architectures |
Structural * | Enterprise |
Behavioral * | Stream Processing Design |
Private: | Distributed systems design |
Parallel programming | Database |
Object generation |
The three patterns with an asterisk are the first group of pattern designs from the Gang of Four book. And then the rest came up. At a minimum, it is useful to know them, because. unknowingly, developers use them in their code. Or offer to remake the code to match the pattern. It would be nice to know these templates (useful for interviews): Singleton, Observer, Template method, Prototype, Adapter.
6. Principles of SOLID, DRY, KISS, YAGNI, BDUF, APO, Occam’s Razor
Excellent article on Habré about the principles and on the wiki about the principle Peter (the project is too complex to understand even for its developers).
YAGNI: You Aren’t Gonna Need It | SOLID: |
DRY: Don’t Repeat Yourself | S) Single-responsibility principle |
KISS: Keep It Simple, Stupid | O) Open–closed principle |
BDUF: Big Design Up Front | L) Liskov substitution principle |
APO: Avoid Premature Optimization | I) Interface segregation principle |
Occam’s razor: do not multiply things unnecessarily | D) Dependency inversion principle |
7. Vulnerabilities in the code
An equally excellent article about vulnerabilities on Habré and a basic article on wiki.
Usually, security audits are done by pros of the finished product, including the source code. But even with code review, you should be wary if the value of the text field is passed from the page, SQL is glued from it and a query is directly executed in the database, and not a parameterized query is used.
SQL injection | Sanitizing dangerous code in HTML |
Passwords in plain text | Bookmarks in code |
Data insecurity (html code, cookies, API) | No or weak encryption |
Authorization and authentication | Log manipulations |
XSS injection of malicious code | Insecure Cookies |
Information leak |
8. Code rot: causes
https://en.wikipedia.org/wiki/Software_rot
Environmental change | Rarely updated code |
Disposability | Internet connection |
Unused code |
9. Dark patterns
https://ru.wikipedia.org/wiki/Dark_patterns
https://en.wikipedia.org/wiki/Dark_pattern
This applies more to UX / UI design (you can’t deceive the user and give one for the other). But if you notice this in the code, you should pay attention.
trick questions | Hidden payments |
Goods toss | Swap bait |
Trap | Emphasis on guilt |
Stuckerberging personal data | Disguised banners |
Techniques that do not adequately compare prices | Forcing a subscription |
distraction | Spam friends |
10. Static code analysis
https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
There are many tools for code analysis: [фронт]but for [C#]. As standard: Linter for Javascript/Typescript; SonarQube, ReSharper, Visual Studio for C#.
BONUS!!! View Poll Results: How do you code review?
Survey available link.
1 of 7: Top 5 Benefits of Code Review? | 5 of 7: How do you code review (how do you find out, do it right away or put it off for the evening, how do you interact with the committer, your life hacks and approaches). |
2 of 7: What do you check in the code review process? | 6 of 7: What are your suggestions for improving the code review? |
3 out of 7: How much MR do you average per day? | 7 of 7: Leave any information (your name, contacts, etc.) – HIDDEN |
4 out of 7: How much time do you spend on code review per day on average? |
Screenshots with survey results
Presentation available link. Thank you for your attention!
