To ensure code quality and compliance, follow the code review guidelines for developers and reviewers.

Developers

General

  • My code compiles:
    • On command line (Ant)
  • Client and server start without errors
  • I have tested my code
  • My code includes unit tests
    • If possible and reasonable both unit test + live test
  • My code does not break existing tests:
    • I have executed all test suits locally:
      • bin/build.sh junit
      • bin/build.sh junit.live.localhost
    • No failed flaky live tests are observed in the build-server

Code style & cleanup

  • The code follows the coding standards:
    • New code must follow the coding standards
    • Don’t change the formatting/coding style of the existing code
  • I have eliminated unused imports:
    • This can be done automatically via IDE (Save action)
  • My classes are free of warnings that cause complication errors
  • I checked my code for accidental changes (and reverted them):
    • As result of an automatic cleanup action or formatting of the IDE
    • Accidentally inserted empty lines, added white space etc...
    • This can be easily done with a git diff tool (e.g. SourceTree, IDE) prior to commit
  • My code is tidy:
    • Correct indentation & formatting
    • No commented-out code
    • No leftover test methods
    • No unused methods
    • No spelling mistakes in comments
    • No debug log output and System.out.println
    • No hardcoded, development only things in the code
    • The code must look “nice”

For this step, please regard our code style rules for java and our code style template for code formatting.

Code documentation

  • My code includes Javadoc where appropriate:

    • On the class header: A short description (at least one sentence) what’s the purpose of the class
    • On each field of the class if it is not self-explanatory
    • On methods if its purpose is not self-explanatory
    • Avoid documenting method parameters, return types and exceptions where possible
    • Focus on documenting the functionality/purpose of the method instead
    • Avoid documentation, which just rephrases the method/field name:
      • “Sets the price”, “Get the price”, etc
    • Remove empty documentation tags (e.g. added automatically by IDE):
      • @return
      • @param price

  • My code contains code documentation where necessary (to understand the logic)

  • Is my code understandable?

  • Is my code maintainable?

  • Ask yourself:

    • “Would I understand my code as a new developer starting at censhare?”
    • “Will I still understand my code when looking into it after 2 years?”

  • Sometimes rewriting complex code to make it easy to understand is better than trying to document the complexity

Architectural

  • I have considered proper use of exceptions:
    • Handle (catch) exceptions at a higher level (business logic level)
    • When catching exceptions:
      • Prefer re-throwing the same exceptions over wrapping into another exception
      • Log the exception with stack trace and context information: What has failed and on which object (asset)?
      • Don’t ignore exceptions (catching without any action or log messages)
  • I have checked for existing methods/utilities before implementing new ones:
    • Look into existing implementations of similar business logic
    • Ask colleagues for code examples and best practices
  • My code is consistent with the current design/architecture?
    • Ask/talk with Seniors and Software/System architects in the Architecture Quest. For more information check the processes of Team Architecture
  • I didn’t try to circumnavigate existing system/architectural limitations “silently”:
    • Don’t take them for granted/“as is”
    • Notify Product owners, seniors, Software architects about the limitations
    • Use TODO comments in the corresponding code only if it is really necessary. That is in cases when there is nothing that can be done to fix the limitation and an enabler has to be planned for that matter. You should be aware that too many TODOs on the code affect our product’s value.

Logging

  • I have made appropriate use of logging
    • Log important messages on INFO level, less important on <= FINER
  • Remember: Messages on level >= FINE are visible on production systems!
    • Avoid logging within loops or within frequently recurring tasks
    • Log what is done (which business logic is called)
    • Log with context information/parameters (e.g. ID of asset)
    • Remove “debug” log messages, e.g. “####” prefixes, etc
    • Don’t do expensive string concatenations in logger calls; at least guard them with: if (logger.isLoggable(Level.INFO)) ...
    • Don’t log large objects like asset XMLs or JSON structures

Side effects

  • Do my changes have any side effects on existing functionality?
    • Does it break any existing functionality?
    • Does it make existing installations/configurations incompatible?
    • Run tests! But also ...
    • Think about it!

Environment

  • How does my code behave in other environments?
    • Required resource assets not installed
    • Required features not defined
    • Executed by users with restricted permissions
    • Executed by users with restricted (domain) visibility
    • Accessed/loaded assets not visible
  • Do I expect/avoid null pointer exceptions in such cases?

Performance and security

  • Was performance and memory consumption considered?
    • Ask yourself: “What happens if my code will be executed on 1 million assets?”
  • Was security considered?
    • Server always must double-check the user permissions
    • Don’t trust the client
  • Does the code release resources? (HTTP connections, DB connection, files, etc.)
  • Is my code thread safe and does it consider possible deadlocks?
  • Can my code run into an endless loop/recursion?
  • I have considered possible null pointer exceptions

Git

  • Always include YouTrack task ID and the Tracker ID (if a ticket exists) into each commit message * Makes it searchable
  • Try to package your final commits to the master branches (e.g. develop, release/20xx.x) in as few commits as possible
  • Move work not directly related to your task into own commits
    • e.g. “by the way” refactorings, optimizations, documentation ...
  • Prefer Git rebasing over merging
    • To keep history clean (avoid merge nodes)
    • While working on local (not pushed!) branch

Reviewers

Main points

  • The code meets the business requirements
  • The code looks “nice” and tidy
  • No Eclipse/IntelliJ warnings in newly written classes
  • The code is understandable and will be in 2 years
  • Unit tests are present and correct
  • Any security concerns have been addressed (e.g. permission checks)
  • Performance was considered
  • Scalability was considered
  • Comments are comprehensible and neither too numerous nor verbose
  • At least minimal JavaDoc present
    • On class header, on commands, on public (API) methods

Additional points

  • The code considers possible null pointer exceptions
  • The code considers “nonstandard” environments and edge conditions
    • e.g. missing resources (resource assets, feature definitions), limited domain visibility
  • Exceptions have been used appropriately
  • Repetitive code has been factored out
  • Frameworks have been used appropriately
  • Command classes have been designed to undertake one task only
  • Common errors have been checked for

Additional points

  • Potential threading issues have been eliminated where possible
  • The functionality fits the current design/architecture
  • The code is unit testable (not always possible; at least live test is present)
  • The code avoids unjustifiable static methods/blocks wherever possible
  • The code complies to coding standards
  • Logging used appropriately (proper logging level and details; not too much, not too less)
  • The code does not “reinvent the wheel”
  • The code does not have any side effect on existing functionality (at least think about it, but of course no reviewer knows the whole system)