Code and System Design Review Checklist

When writing code, we need to review our own code and other's code as well as software system design and architecture. In this article, we try to share few important notes regarding code and system review.
This checklist is mostly for java backend development. But can also be applied to other technology stacks.
The checklists/notes will help developers ensure better code and system architecture.

Category/Area of Review:

1. General
2. Clean Code & Code style
3. Security
4. Performance
5. Logging and Tracing
6. Concurrency
7. Error Handling
8. Maintainability & Testability
9. Domain(Business)
10. Architecture
11. Scalability
12. Reliability & Resiliency
13. Design pattern
14. PCI DSS(FinTech)
15. REST API design

General:
Use checked exceptions for recoverable conditions and runtime exceptions for programming errors
Try to use global exception handling and handle common Business and technical error response
Never ignore exceptions. Don't overlook the catch block.
Return empty arrays or collections, not nulls.
Minimize the scope of local variables for earlier GC.
Always override hashcode when overriding equal.
Always override toString
Use marker interface to define the type
Use an executor thread pool for tasks and thread instead of unlimited thread creation.
Use the BigDecimal valueof method for the string to bigdecimal/double conversion, otherwise, you will face a precision issue.
Try to avoid string literals at business logic check. Use enum or constant for maintainable code.

Bad:
if(status = "SUCCESS")
Good:
if(status = EventStatus.SUCCESS)
Throw Exceptions rather than Return codes in case of business and technical error.

Bad:
private String checkInput(Request request){
if(something wrong)
return "FAILED";
}
Good:
private String checkInput(Request request){
if(something wrong)
throw new BusinessErrorException(int code, String message)
}

Clean Code:
Use Intention-Revealing Names for variable

Bad:
void validate(String input)
Good:
void validateCardNumber(String cardNumber)
Pick one word per concept.
Use Solution/Problem Domain Names
Classes should be small!
Functions should be small!
Do one thing in a function.
Don't Repeat Yourself (Avoid code Duplication).
Explain yourself in code(write why in code not what)
Make sure the code formatting is applied(Can use tools)
Each method should do a single task. Don't mix business logic and network calls with the same method. Try to make the method unit testable.

Bad:
public sendSms(){
// code for validation
if(mobile is valid mobile)
// code for sms generation
String smsBody = "Some Text"+variable+"text".
// network call to telco server
restTemplate.exchante(url,mobile,smsBody);
}
Good:
public processSms(){
validateMobile();
String smsBody = generateSmsBody(inputs);
sendSms(inputs)
}
private void validateMobile(String mobileNo){}
private String generateSmsBody(Inputs.){}
private sendSms(){}

Security:
Check access control or authorization besides authentication.

Bad:
Only verify the JWT token
Good:
Verify token and check use have authority to access that resource.
All change event applications should be auditable(who performed this operation from which device and IP)

Bad:
account(name,status,createdDate)
Good:
account(name,status,createDate,createdBy,lastModifiedDate,lastModifiedBy)
Use password as an array of characters instead of String so that no one can get it from heap dump.
Make class final if not being used for inheritance.
Input into a system should be checked for valid data size and range and check mandatory input fields(boundary conditions)

Bad:
TxnRequest{
String fromAc;
String toAc;
BigDecimal amount;
}
Good:
TxnRequest{
@NotBlank(message="From AC Can't be blank.")
String fromAc;
@NotBlank(message="To AC can't be blank.")
String toAc;
@notnull(message="Amount can't be null.")
@Positive(message="Amount should be positive")
BigDecimal amount;
}
Avoid sensitive data logging(like pin, password, card info)

Bad:
log.info("User:{}, Password{}",request.getUserName(),request.getPassword());
Good:
log.info("UserName:{}, Password:****",request.getUserName())
Purge sensitive information from exceptions (exposing file path, internal system configuration)
Be careful about SQL injection when DB queries.
Check the API response fields. Is there any extra data or sensitive data are shared with the public?

Bad:
Ac Balance response:
{"account":"10421020025347",
"balance":550.00,
"modifiedBy":"bank-checker-001",
"userId":456,
"customerInfo":{.}}
Good:
{"account":"10421020025347","balance":550.00}
Define wrappers around native methods (not declare a native method public).
Make public static fields final (to avoid caller changing the value)
Try inter-service communication in a secured way(Implement SSL when service to service call)
Separate public and private/internal API paths so that DevOps team can implement infra-level security and filter.

Bad:
api/v1/all public/admin/internal path,
Good:
api/v1/public path
api-admin/v1/admin related path
api-internal/v1/interapi
In the case of microservice try to use a central auth server
Check about authentication and authorization network call overhead in the case of a distributed system.
Never use default credentials at production. Especially for system/infrastructure-related services. (DB, Cache, Auth, API GW, HTTP server, 3rd party library)
Encrypt or one-way hash for OTP and other credentials.
Ensure REST API security.

Performance:
Try to keep synchronized section small operation(CPU/network/memory)
Avoid string literal concatenation back end component. Try to use a string builder.

Bad:
String fullMessage = "";
for(Result result : resultList){
fullMessage = fullMessage+result.getMessagr();
}
Good:
StringBuilder fullMessage = new StringBuilder();
for(Result result : resultList){
fullMessage.append(result.getMessage()));
}
Avoid creating unnecessary objects.
In case of network call use a connection pool, thread pool, socket pool.
Profile DB query and check high search/read query happen on an indexed field.
Release resources (Streams, Connections, etc) in all cases.
Careful about ORM N+1 query and use Entitygraph to avoid N+1 query
Always think about cache.
In a distributed system(SOA or Microservice) always develop stateless service.
Try to develop as many Asynchronous processes using JMS.
Think about IO latency and CPU usage. If the system is an IO incentive try to use the NIO library.
Check if any unused library goes into the production build.
In the case of an in-memory store avoid full object serialization and use custom serialization to save memory. Think about JSON vs Messagepack vs protobuf.
Try to avoid select * if not needed at DB query.
Try always index-only scan, select the field which is indexed. The most important and readable field should be indexed. We can also use composite indexed-key.

Logging &Tracing:
Maintain proper log level
Use placeholder (LOG.error("Could not details: {}", something, exception)) and never String concatenation (LOG.error("Could not details: " + something, exception))
Don't trace excessive logs.
In the case of Distributed Systems (SOA, Microservice) try to use Distributed tracing (Spring cloud sleuth, ELK, Zipkin)
Don't log Sensitive Information
Log User Context
In the case of centralized logs use common trace id and service name.

Concurrency:
Avoid excessive synchronization for thread safety. Try to avoid sharing resources in case of a multithreaded environment.
Avoid member variables when in the case of a singleton object or bean.
Always synchronize share resources and also try to avoid share resource
Use concurrent hashmap instead of Synchronize HashMap
Use HashMap or HashSet instead if TreeMap/Set when ordering is not important. As time complexity O(1) vs O(logn)
Always think about concurrency in the case of back-end service.
When multiple users/threads update the same data, try to implement a lock(optimistic or pessimistic). For example account balance, ticket reservation, product stock.

Error Handling:
Reply consistent error response to the client.
Handle proper error code(401,404,400 and 500)
Use custom error code for the business logic errors.

Typical Error Categories

• Business logic error
• Technical Error
• Upstream Service Error
• Common Error/Runtime ErrorArchitecture:Follow twelve-factor app(https://12factor.net)Aware about distributed system fallacy (https://en.wikipedia.org/wiki/Fallacies_of_distributed_computing)For the data layer we can follow lambda architecture.Try to implement event-based infrastructure for scalability.Follow 12 architecture principle

1.N+1 Design. Never less than two of anything and remember the rule of three
2.Design for Rollback. Ensure you can roll back any release of functionality
3.Design to be disabled. Be able to turn off anything you released
4.Design to be monitored. Think about monitoring during design. Not after.
5.Design for multiple Live sites. Don't box yourself into one site solution
6.Use mature technology. Use thing you know work well
7.Asynchronous design. Communicate synchronously only when absolutely necessary
8.Stateless Design. Use state only when the business return justifies it .
9.Scale Out NOT Up. Never rely on a bigger or faster system.
10.Design for at least two axes. Think one step ahead of your scale beads.
11.Bue when Non Core. If you aren't the best at building it and it doesn't offer competitive differentiation, buy it.
12.Commodity Hardware. Cheaper is better most of the time.
Design Pattern:
SOLID
DRY
KISS
Creational Pattern(Singleton,Factory,Builder,Adapter)
Behavioral Pattern(Strategy, Chain Responsibility, Observer).

Scalability:
Make the system elastic or easily scalable.
Make the service stateless for horizontal scaling.
DB Sharding
DB Partition
DB replication
Use read replica for an independent read operation.
Cache high read and low write operation.
Monitor DB read and write query ratio.
Reduce network call overhead (use socket pool or gRPC).
Check DB network call round trip and always try to reduce it.
Address the c10K problem for network communication.

Reliability & Resiliency:
Handle Timeout in case of all network call.
Implement Circuit Breaker Pattern.
Implement Bulkhead pattern
Implement idempotent operation
Use a fail-first mechanism when the system is really broken.

Maintainability & Testability:
How much test coverage does your system have?
Unit, Integration, and System Testing coverage.
Proper CI/CD pipeline.
Do as much automation.
Implement proper production deployment test strategy( canary, AB testing)

Monitoring/Observability:
Monitor your system's health and resources.
Database Memory and disk size
Application and DB server CPU, memory and Load average, and IO
Application latency and throughput.
Application resource(JVM memory, thread, thread pool, connection pool, queue, JMS memory)
Isolate Logging, Tracing, and metric analysis.
Monitor your business performance.

Business Domain:
Business Logic implemented properly at the codebase
Service properly isolated against the domain and bounded context.
Try to follow the domain-driven design.

PCI DSS:(For FinTech):
12 requirements:
1.Install and maintain a firewall configuration to protect cardholder data
2.Do not use vendor-supplied defaults for system passwords and other security parameters
3.Protect stored cardholder data
4.Encrypt transmission of cardholder data across open, public networks
5.Use and regularly update anti-virus software or programs
6.Develop and maintain secure systems and applications
7.Restrict access to cardholder data by business need to know
8.Assign a unique ID to each person with computer access
9.Restrict physical access to cardholder data
10.Track and monitor all access to network resources and cardholder data
11.Regularly test security systems and processes
12.Maintain a policy that addresses information security for all personnel
REST APIDesign:
Use kebab-case for URLs

Bad: /systemOrders or /system_orders
Good: /system-orders
Use camelCase for Parameters

Bad: /system-orders/{order_id} or /system-orders/{OrderId}
Good: /system-orders/{orderId}
Plural Name to Point to a Collection

Bad: GET /user or GET /User
Good: GET /users
Keep Verbs out of Your Resource URL

Bad: POST /updateuser/{userId} or GET /getusers
Good: PUT /user/{userId}
Use Verbs for Non-Resource URL or specific operation

POST /alerts/245743/resend
Use camelCase for JSON property

Bad:
{
user_name: "Mr Rahim"
user_id: "1"
}
Good:
{
userName: "Mr. Karim"
userId: "1"
}
Don't use table_name for the resource name

Bad: product_order
Good: product-orders
This is because exposing the underlying architecture is not your purpose.
Use API version

Good: http://api.domain.com/v1/shops/3/products
Accept limit and offset Parameters

GET /shops?offset=5&limit=5
Don't Pass Authentication Tokens in URL

Bad:
GET /shops/123?token=some_kind_of_authenticaiton_token
Good:
Instead, pass them with the header:
Authorization: Bearer xxxxxx, Extra yyyyy
Use the Relation in the URL For Nested Resources

GET /shops/2/products : Get the list of all products from shop 2.
GET /shops/2/products/31: Get the details of product 31, which belongs to shop 2.
CORS

Do support CORS (Cross-Origin Resource Sharing) headers for all public-facing APIs.
Consider supporting a CORS allowed origin of "*", and enforcing authorization through valid OAuth tokens.
Avoid combining user credentials with origin validation.
Security
Enforce HTTPS (TLS-encrypted) across all endpoints, resources, and services.
Enforce and require HTTPS for all callback URLs, push notification endpoints, and webhooks.
Reviewer & reviewee's behavior and attitude when reviewing other'scode:
Be kind
Accept that many programming decisions are opinions. Discuss trade offs, which you prefer, and reach a resolution quickly.
Ask questions; don't make demands. ("What do you think about naming this:user_id?")
Ask for clarification. ("I didn't understand. Can you clarify?")
Avoid selective ownership of code. ("mine", "not mine", "yours")
Avoid using terms that could be seen as referring to personal traits. ("dumb", "stupid"). Assume everyone is intelligent and well-meaning.
Be explicit. Remember people don't always understand your intentions online.
Be humble. ("I'm not sure-let's look it up.")
Don't use hyperbole. ("always", "never", "endlessly", "nothing")
Be careful about the use of sarcasm. Everything we do is public; what seems like good-natured ribbing to you and a long-time colleague might come off as mean and unwelcoming to a person new to the project.
Consider one-on-one chats or video calls if there are too many "I didn't understand" or "Alternative solution:" comments. Post a follow-up comment summarizing one-on-one discussion.
If you ask a question to a specific person, always start the comment by mentioning them; this ensures they see it if their notification level is set to "mentioned" and other people understand they don't have to respond.

Resources:
https://owasp.org/www-pdf-archive/OWASP_Code_Review_Guide_v2.pdfhttps://dzone.com/articles/java-code-review-checklisthttps://www.codementor.io/blog/code-review-checklist-76q7ovkaqjhttps://owasp.org/www-pdf-archive/OWASP_SCP_Quick_Reference_Guide_v2.pdfhttps://dimikcomputing.com/course/clean-code-online-course/https://stackoverflow.com/questions/7186204/bigdecimal-to-use-new-or-valueofhttps://www.geeksforgeeks.org/use-char-array-string-storing-passwords-java/https://betterprogramming.pub/10-essential-tips-for-writing-secure-rest-api-e297990d48c5https://cloud.google.com/architecture/application-deployment-and-testing-strategieshttps://docs.gitlab.com/ee/development/code_review.html