P0 Checklist

This list is a practical, minimal, non-exhaust checklist that contains the P0 area to look for when reviewing a PR. This can be seen as a best practice list to both PR reviewer and PR author.

Contributing

Given this list is of the top priority, we would like to keep it minimal and consensual. To make it to this list, the corresponding rule must be thoroughly discussed and reviewed by the code review committee.

Unit Testing

• All Go code should have at least 85% test coverage.

Difference Repository can have higher requirement. For e.g. PW enforce 90% coverage.

• Use Test Table Style  to write your test whenever possible.

Table-driven test can help to reduce redundancy and improve readability

• Do not assert full error message in test.

Error msg is not a reliable way of identifying a error and can change without notification

Panic

• Pay extra attention to unexpected panic, esp nil reference (NPE). It is the #1 mistake one should try to avoid.

• Don’t use keyword panic, except for truly exceptional cases.

This one has been documented in the several well known reference

• Don’t use panic for normal error handling 
• Reserves panic for the truly exceptional  So when to use actually panic keyword? This could be a good reference When to use panic 
• Initialization Failures - a package level variable or init function fails to initialize properly, or during server start time for example, app.go
• When error handling isn’t possible. For example, a config error that prevents the application from continue running the rest

Pointer

• Use a pointer type to represent a nullable value.

• Use pointer receiver by default

Choose pointer vs value receiver 

Error handling

• Avoid golang native “errors” library.

Golang errors lib does not establish stacktrace, which is crucial for debugging. Use github.com/pkg/errors instead.

• Avoid return nil, nil for your function when possible.

Keeping an assumption of if err is nil, then the response should not be nil helps reduce the unnecessary nil check, keeping the code leaner

• Avoid discard errors using _ variables. If a function returns an error, check it.

See go code review wiki 

• Always use meaningful error messages to provide clarity.

Error messages should be clear and concise to help developers understand the root cause of the issue. Example: failed to perform action A while doing action B for reason C, with reason being optional.

• Do not use error strings to control your flow of biz logic, if possible.

  Error msg can change without notice and is only for debugging purposes. Not a reliable indicator.

• Do not wrap errors unless you want to change the nature of the error.

For example, forcing a retryable error to be non-retryable error can be achieved through wrapping. Unnecessary wrapping makes debugging harder and is error-prone.

• Do wrap errors if you are handling an error that is thrown by a non circle lib.

For example, json.Marshall error should be wrapped, to establish the stacktrace

Logging

• Use PCG logger if possible

• Conform to circle logging level standard 

• If the errors are already caught, don’t log it again.

  • For http server endpoint error, the common logging middleware in PCG will log all errors returned.
  • For error throw out by the worker, PCG will log it as well.

Microservice - Resource layer

• All resources (endpoint) should be created using CircleResource.

CircleResource  includes a version for every resource and integrates error handling and success body marshaling through the CircleResource.Handle()

Microservice - Repository layer

• Use PCG QueryBuilder to build query whenever possible.

Use PCG Query Builder to easily write standard queries. Avoid hand-crafting query if possible. If PCG Query builder does not support a use case or method, please create a ticket under the platform-common-go epic.

• Only use PCG Handle to execute query

When it comes to executing SQL queries, always use PCG Handle, similar to the JDBI Handle. The handle will make sure binding and db transactions are properly managed. There is NO exception to this rule.