Manual Code Review Csaba

Brknyi commented 2 years ago

One part of the library is get reviewed by with a manual code review checklist.

Brknyi commented 2 years ago

Implementation

[x] Does this code change do what it is supposed to do?
[x] Can this solution be simplified?
[x] Does this change add unwanted compile-time or run-time dependencies?
[x] Was a framework, API, library, service used that should not be used?
[x] Was a framework, API, library, service not used that could improve the solution?
[x] Is the code at the right abstraction level?
[x] Is the code modular enough?
[x] Would you have solved the problem in a different way that is substantially better in terms of the code’s maintainability, readability, performance, security?
[x] Does similar functionality already exist in the codebase? If so, why isn’t this functionality reused?
[ ] Are there any best practices, design patterns or language-specific patterns that could substantially improve this code?
[ ] Does this code follow Object-Oriented Analysis and Design Principles, like the Single Responsibility Principle, Open-Close Principle, Liskov Substitution Principle, Interface Segregation, Dependency Injection?

Brknyi commented 2 years ago

Dependencies

[x] If this change requires updates outside of the code, like updating the documentation, configuration, readme files, was this done?
[x] Might this change have any ramifications for other parts of the system, or backward compatibility?

Brknyi commented 2 years ago

Logic Errors and Bugs

[x] Can you think of any use case in which the code does not behave as intended?
[x] Can you think of any inputs or external events that could break the code?

Brknyi commented 2 years ago

Error Handling and Logging

[ ] Is error handling done the correct way?
[ ] Should any logging or debugging information be added or removed?
[ ] Are error messages user-friendly?
[ ] Are there enough log events and are they written in a way that allows for easy debugging?

Brknyi commented 2 years ago

Security and Data Privacy

[x] Does this code open the software up for security vulnerabilities?
[x] Are authorization and authentication handled in the right way?
[x] Is sensitive data like user data, credit card information securely handled and stored?
[x] Is the right encryption used?
[x] Does this code change reveal some secret information like keys, passwords, or usernames?
[x] If code deals with user input, does it address security vulnerabilities such as cross-site scripting, SQL injection, does it do input sanitization and validation?
[x] Is data retrieved from external APIs or libraries checked accordingly?

Brknyi commented 2 years ago

Performance

[x] Do you think this code change will impact system performance in a negative way?
[ ] Do you see any potential to improve the performance of the code?

Brknyi commented 2 years ago

Usability and Accessibility

Brknyi commented 2 years ago

Readability

[ ] Was the code easy to understand?
[ ] Which parts were confusing to you and why?
[ ] Can the readability of the code be improved by smaller methods?
[ ] Can the readability of the code be improved by different function/method or variable names?
[ ] Is the code located in the right file/folder/package?
[ ] Do you think certain methods should be restructured to have a more intuitive control flow?
[ ] Is the data flow understandable?
[ ] Are there redundant comments?
[ ] Could some comments convey the message better?
[ ] Would more comments make the code more understandable?
[ ] Could some comments be removed by making the code itself more readable?
[ ] Is there any commented out code?

Brknyi commented 2 years ago

Testing and Testability

[x] Is the code testable?
[ ] Does it have enough automated tests (unit/integration/system tests)?
[ ] Do the existing tests reasonably cover the code change?
[ ] Are there some test cases, input or edge cases that should be tested in addition?

Brknyi commented 2 years ago

Experts Opinion

[ ] Do you think a specific expert, like a security expert or a usability expert, should look over the code before it can be committed?
[ ] Will this code change impact different teams? Should they have a say on the change as well?

BME-MIT-IET / iet-hf-2022-holnapejfeligszerintemmindlepjunkbe