Skip to content

Development Guidelines

Jump to bottom
Sean Flanigan edited this page Jun 9, 2016 · 1 revision

(DRAFT)

These guidelines should be kept in mind when developing Zanata, and when reviewing changes (pull requests) for Zanata.

Review Considerations for Pull Requests

  • Test coverage
  • Static analysis, eg FindBugs
  • Correctness
  • Is the code correct? How do we know? Are the tests adequate?
  • Clarity
  • Is the code readable and understandable?
  • Maintainability
  • Will this make maintenance harder than it should be?
  • Performance and scalability
  • Will this perform well when N is large? (Where N is number of users, or projects, or documents, or strings, or requests per minute, etc.)
  • Will this change cause existing operations to become significantly slower?
  • Cluster safety
  • If application is running in a cluster, how will other nodes see this data?
  • Everything in the session should be Serializable
  • Security
  • Thread safety
  • Watch out for shared mutable data

General

  • Consider the code reviewer
    • Don't make trivial changes which make long diffs but don't improve anything
    • Don't reformat for the sake of it
      • If you need to reformat, use a separate commit
    • Don't amend long commits which may have been reviewed already - just add another commit
  • Consider warnings from tools like FindBugs, but don't just do everything they say
  • Prefer immutable data where possible: (We may need to choose a good functional programming library. Functional Java?)
  • Make sure your pull request merges cleanly:
    • This allows QA to test the merged version.
    • If there are merge conflicts, the dev is usually the best one to fix them.
  • Consider technical debt - are you reducing it or increasing it?
  • Consider performance and scalability in APIs, in designs and in coding
    • Are you sure it will fit in memory? Can you stream it?
    • Will this enhancement incur performance costs for all requests, even though only some requests will benefit?
    • Don't use exponential algorithms
      • Try to minimise iteration - better data structures may help (eg look up in a map instead of iterating over a list)
  • Use static analysis; eg keep FindBugs clean!
  • If rewriting something (code, pages, docs, tests), check the old version so that we don't "forget about" existing features, docs or tests
  • If you need to download a build dependency (eg an app server), make sure you cache it outside the build directory (eg ~/.m2 or ~/Downloads, not ./target) so that mvn clean won't destroy it, but be careful how you cache it to avoid staleness.
  • Don't use enums simply to hold string constants - use a class with public static final fields!

Seam-specific

  • Never inject lower-scoped beans into larger-scoped beans
    • eg a Session bean into an Application bean
    • this can lead to thread safety problems

CDI (read the weld manual)

  • Don't use @Singleton: use @ApplicationScoped
  • If you miss flash scope, use a conversation or other DeltaSpike scope
  • @SessionScoped beans should generally be @Synchronized for thread safety
  • Tip: Code which uses database needs @Transactional
  • Tip: Event handlers with "@Observes(during=TransactionPhase.AFTER_SUCCESS)" will need @Async if they use @Transactional

Docs

  • Update docs if appropriate
  • Remember to update the release notes for infrastructure changes (Straightforward JIRA issues will be listed as one-liners during the release process)

Database

  • Use named queries
  • Use one operation per liquibase changeset
  • For long operations, check precondition (eg index does not exist) first
    • This will help sysadmins, in case the operation times out

Logging

  • Don't use System.out/err.println - generally prefer log.debug
  • Learn to configure the logging framework, eg to enable DEBUG

Exception handling

  • Never catch and ignore, eg try { something(); } catch (Exception e) {/* VERY BAD*/}
  • Catch and handle, or throw another (chained) exception
  • Don't log an ERROR and throw too
    • logging at DEBUG is okay
    • logging as WARN may be used if you suspect third party code will catch the exception without logging)

JavaScript code

  • Don't paste .js into our source tree; use webjars.org or similar approach to package .js as a Maven dependency

UI code

  • test UI changes on both Firefox and Chrome
  • all UI code must be internationalised, with Jenkins jobs to:
    • extract POT, upload to translate.zanata.org
    • download PO from translate.zanata.org, commit to source tree

Tests are not optional

  • Unit tests are preferred in most cases
    • otherwise, write a system test
  • Especially important for dynamically typed code, including JavaScript and Groovy

Mock testing

  • If you need a dozen mocks, something is wrong with your test or with the code - refactor!
  • spy(objectUnderTest) should never be needed: refactor!
  • Prefer when(mock.methodCall()).thenReturn(result), not doReturn(result).when(mock).methodCall()

Selenium Tests

  • Don't use implicit wait
  • Don't even use explicit "wait for condition" if you can help it: "timed out" errors aren't helpful
  • Try to use waitForPageSilence(), then just assert the condition you want to check
    • This should lead to better error messages
    • Consider improving waitForPageSilence() if you need it

Keep Zanata server together

If some code is deployed with Zanata server and maintained by Zanata team:

  • it should not be an external dependency!
  • the code should be in the same repo, tagged together, tested together
  • it should be built from source into zanata.war automatically (eg by Maven)
    • this makes integration and testing far easier
    • important for continuous integration and continuous delivery

New technologies and dependencies

Don't introduce major new technologies without considering:

  • reliability
  • cross-platform support (RHEL, Fedora, Mac, Windows)
  • compatibility with other technology choices (or future choices)
    • eg Lombok vs IDEs
    • eg Lombok vs Java 8, 9...
  • how to build and release (currently via Maven)
    • this includes Jenkins setup - if you break it, you bought it
  • how to deploy (currently via MEAD)
  • i18n (for UI technologies)
  • security
  • unit testing and system testing
  • maintainability of code
  • consider the "hit by a bus" factor
  • the rest of the team needs to be able to work with the code

Third party libraries

  • Consider parentage/governance (will it be maintained long term?)
  • Check for compatible licence
  • Check code quality, tests, frequency of release (will it keep up with new tech, eg Java 9 bytecode?)
    • warning signs: ignored pull requests, ignored bugs
  • Be especially careful with libs which manipulate code/bytecode
  • Be prepared to keep the library up to date
  • For client-side code: is the dependency in Fedora, and up to date, or will we need to add it ourselves?

Zanata wiki

Clone this wiki locally