Sign in

Creating a new feature

Create a plan of attack

Before starting to code think about the following.

  • Is the issue issue clearly describing the changes that is needs to be done to the system?
  • Do I know where these changes needs to be implemented?
  • Is there any changes that needs to be implemented in advance of the changes that issue issue is describing?
  • Do I see any consequences by implementing these changes that have not been documented in the issue issue?
  • Which aspects of the implementation are strictly needed to solve the issue, and which can be implemented later? Avoid premature engineering.

To help ourselves implement the correct thing we (the team) will create a plan for how to solve a certain issue.

This plan should include:

  • What changes to make
  • Where to make them
  • The tests that will be needed to ensure that the system works
  • An estimate in hours of the time needed to complete the issue
  • Any information that you need to gather to complete the issue

It is recommended to use a whiteboard and other team members to create the plan before documenting it on the issue.

We do this to make sure we do not have scope creep inside a single issue and to avoid over engineering and gold plating.

Create a feature branch

git checkout -b feature/XYZ-123-new-stuff

The feature branch belongs to the developer who created it, that developer can rewrite history on the branch (eg: using git rebase). Should other developers be using that branch they need to coordinate with the branch owner.

Push the feature branch to the project GitHub repository

git push

Do work

  • Write tests, documentation and code.
  • Create milestone commits as you like, think of milestone commits as your personal undo tree.
  • Push often (You might get hit by a buss on the way home).

Prepare for review (Pull Request)

  • Squash milestone commits into "meaningful" commits. Usually one commit per feature branch, if you need more you are probably building more than one feature. All commits should pass all tests. To squash all commits in a branch that was based off master.

    git rebase --interactive master

    This will bring up your configured editor with content similar to this.

    pick f7f3f6d changed my name a bit
    pick 310154e updated README formatting and added blame
    pick a5f4a0d added cat-file
    
    # Rebase 710f0f8..a5f4a0d onto 710f0f8
    #
    # Commands:
    #  p, pick = use commit
    #  r, reword = use commit, but edit the commit message
    #  e, edit = use commit, but stop for amending
    #  s, squash = use commit, but meld into previous commit
    #  f, fixup = like "squash", but discard this commit's log message
    #  x, exec = run command (the rest of the line) using shell
    #
    # These lines can be re-ordered; they are executed from top to bottom.
    #
    # If you remove a line here THAT COMMIT WILL BE LOST.
    #
    # However, if you remove everything, the rebase will be aborted.
    #
    # Note that empty commits are commented out

    Replace pick with squash for all the commits except the first one and save the file. This will combine all the changes from the commits into a new commit.

    pick f7f3f6d changed my name a bit
    squash 310154e updated README formatting and added blame
    squash a5f4a0d added cat-file
    
    # Rebase 710f0f8..a5f4a0d onto 710f0f8
    #
    # Commands:
    #  p, pick = use commit
    #  r, reword = use commit, but edit the commit message
    #  e, edit = use commit, but stop for amending
    #  s, squash = use commit, but meld into previous commit
    #  f, fixup = like "squash", but discard this commit's log message
    #  x, exec = run command (the rest of the line) using shell
    #
    # These lines can be re-ordered; they are executed from top to bottom.
    #
    # If you remove a line here THAT COMMIT WILL BE LOST.
    #
    # However, if you remove everything, the rebase will be aborted.
    #
    # Note that empty commits are commented out

    Now run git push --force to update the remote version of the branch

    For more information see Rewriting-History - Git Official Documentation.

  • Create a pull request in the repository in GitHub, assign one or more people as reviewers for your pull request.

Review a Pull Request

  • Do not approve if CI/CD is failing, or "Work in Progress" (WIP) is set.

  • Do not merge on behalf of another developer.

Checklist

General

  • Does the code work? Does it perform its intended function, is the logic correct etc.
  • Is all the code easily understandable?
  • Does the code conform to your agreed coding conventions? Coding conventions will usually cover location of braces, variable and function names, line length, indentations, formatting, and comments.
  • Is there any redundant or duplicate code?
  • Is the code as modular as possible?
  • Can any global variables be replaced?
  • Is there any commented out code?
  • Do loops have a set length and correct termination conditions?
  • Can any of the code be replaced with library functions?
  • Is the code allocating resources irresponsibly?
  • Is it doing exponentially more iterations over collections than necessary?
  • Can any logging or debugging code be removed?

Security

  • Are all data inputs validated (for the correct type, length, format, and range) and encoded?
  • Where third-party utilities are used, are returning errors being handled?
  • Are output values checked and encoded?
  • Are invalid parameter values handled?

Documentation

  • Do comments exist and describe the intent of the code?
  • Are all functions commented?
  • Is any unusual behavior or edge-case handling described?
  • Is the use and function of third-party libraries documented?
  • Are data structures and units of measurement explained?
  • Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like ‘TODO’?

Testing

  • Is the code testable? i.e. don’t add too many or hide dependencies, unable to initialize objects, test frameworks can use methods etc.
  • Do tests exist and are they comprehensive? i.e. has at least your agreed on code coverage.
  • Do unit tests actually test that the code is performing the intended functionality?
  • Are arrays checked for ‘out-of-bound’ errors?
  • Could any test code be replaced with the use of an existing API?
  • Are tests based on an unreliable external resource / API instead of being mocked?

If the reviewer requires changes, make comments on the lines where changes are required and use the "Request changes" functionality in GitHub.

The developer fixes the changes, and the reviewer reviews again until approved.

Merging the code

  • Squash commits made during code review, see Prepare for review (Pull Request) for instructions on how to squash commits in a branch.
  • The developer of the code merges the code into master.
  • The developer and the people who approved the pull request are mutually responsible for the quality of the tests, documentation and code produced by the pull request.