Robert Zaremba, https://scale-it.pl
How we work
Development Team Agreements
This document presents the set of objectives for the development team. It extends the How We Work “affection”.
2. Extreme Programming
We will practice Agile Extreme Programming.
- Weekly sprint planning: Monday morning.
- Daily updates. Every day when you start a work.
As per distributed environment we will do it in the #dev-daily-updates channel.
Everybody, every working day should post there following message:
What I’ve done. What I’m doing / plan to do. What are the blockers. What I found interesting (if any). Update the task board!
- Keep on track with the sprint plan.
Always check the task board (trello) and update once you achieve something. Comment if something is badly designed, doesn’t work, there is a blocker or when something is just great!
Work principles: Be proactive. Measure. Communicate!
- Weekly sprint review. Friday morning.
- Retrospective every 2 weeks.
3. Communication and collaboration
In a distributed environment communication is crucial. Without a good communication we will experience bad process. To ensure a good communication, all team members consent to:
- Be open — always inform your colleagues about your issues, when do you finish your work, when they can count on you. Always inform about non-default schedule.
It’s normal that you have private things to do. But don’t hide it. Others may expect something from you. Let them know that you won’t be available (if it’s something longer).
- Plan — if you have impediments, issues or something which puts the delivery into a risk - plan it. Be proactive. Think about it. Discuss it. Best if you can propose some preliminary solutions.
- Be honest! This is very important. We can’t plan or communicate well if you are not honest. If you have a big problem -- let us know. Otherwise we will fail to make a plan and work as a team. Remember the team performance depends on you.
- If you have a personal issue — talk about it with the team manager. It’s better to solve things together.
- Be responsible.
- Provide daily updates (stand-up). Even if something was discussed, it’s better to repeat yourself in daily update then not doing it at all.
- Even if you didn’t do much - commit and push your changes.
- Every day update the task board. Move tasks or put comments.
Working together with a code.
If some feature development is dependent on other task which is work-in-progress, then don’t hesitate to do it in a single branch. It’s fair (and it’s part of git design) to allow multiple people work on a single branch. This is crucial when you have an important task, and someone wants to collaborate, review our get your updates. Make it easy to allow others to collaborate.
4. Development Flow
- All code resides in repository assigned to the project.
- Follow the tasks.
- You should always have some ongoing task.
- All your ongoing tasks must have estimated due to date.
- Don’t hesitate to create new tasks - put them into the icebox (defined in FAQ below).
- Create a task for every feature / bug / ticket.
- Focus on your current tasks. Whenever you will find new thing to do, just add a task to the icebox section instead of losing a focus of a current task. We will plan it later.
- Communicate early and openly! Start from solution design before coding any complex task and present it. Make a functional review (no code review) for each bigger feature (>= 2 days) early - like in ⅓ of the work, where you will share your design and algorithm.
- Gitlab flow
- All features should be in branch
- Create meaningful names to your branch
- Always assign a reviewer to your code when doing pull-request (PR)
- Keep the features small and create PRs often (ideally 1-3 days, max 4-5 days).
- All packages / projects have to have integrated linters and test framework.
- We start each new API related task with a blueprint design. For the REST we are using the API Blueprint. For other kind of APIS (like GraphQL or a simple package design) we use the the early code blueprint with interfaces. If the API design is not ready, push it and get early review! Say that it’s blocking. We prefer to spend more time on the design rather than spending too much time on rewrite.
- Frontend can use API Blueprint mock server to use it when implementing the frontend features before waiting for the backend.
- Backend should validate the response with the API Blueprint
- We use Snowboard as the rendering engine (documentation) and mock server.
- Make sure your editor has configured editorconfig support. The repositories contain .editorconfig file and it should be well respected.
- All feature / bug / ticket implementations should have corresponding tests. It is required that each new feature will have:
- unit test for all critical computation parts (frontend model and all backend packages)
- module test for backend functionality covering real world scenario.
- All code in the master branch must:
- Pass all tests
- Pass all linters
- Be well structured
- Take care about code quality. Check the sections below:
- What we use to preserve good code style
- What is a filename convention
- Choose wisely about function / variable names. Look below at Code Style
- What's in a name?
- Master branch is untouchable! Only merges from feature / hotfix branch are allowed.
- Implement all features in branches
- You can't rebase master and hotfix branch. You can rebase feature branches.
- Each application should have a README file which provide:
- Explain application purpose
- List all dependencies
- Explain the privacy / security mode (who and how can use it)
- Runtime requirements: can it scale, how it should be run, what are minimum computing resources required (memory, cpu, …).
- Links to the application design documents (if any).
- Installation instructions.
5. Code Review
The overall aim of the code review process is to increase the quality of the codebase.
When you finish implementing a feature make a pull request and ask someone for code review.
- Before doing PR, make source code is well organized and clean
- Add reviewers (minimum 1), best if at least one is knowledgeable about requested changes.
- Tick “Close branch”
- Merge only when all PR tasks are resolved and all responses to comments provided
- Focus on finishing current PR. It should never stay open for a long time.
- If you have questions ask openly
- If you wait - ping people you depend on
- Inform about any obstacles.
- Last, but not least: our methodology is to deliver step by step
- Before merging squash not-relevant commits. In essence in most of the cases you should end up with only one commit. Bitbucket provide a feature for that when merging the PR (image below).
- Merging should be possible only if there is approval quorum between reviewers.
Not 'does it work?' but 'is it easy to understand?' and 'is it simple?'
It is not the responsibility of the code reviewer to ensure that there are no bugs before code is committed, nor even that the code functions at all. These are 100% the responsibility of the person submitting the code. If the reviewer wants to undertake these checks then that is great, but beyond what we expect of code reviewing.
The code reviewer should primarily assess the maintainability of the code submitted, which means asking the following basic questions:
- is it easy to understand? (could it be made easier to understand?)
- does it follow best practices? (below)
- is it well-documented?
- does it follow code conventions?
The enemy of both code maintenance and code robustness is complexity. Your task as code reviewer is to find complexity and complain about it.
The code reviewer can also assess by inspection other important aspects of the code, such as:
- resource efficiency
Caveat: as a code reviewer prioritize comprehensibility over efficiency. In other words, as is often said, we should aim to 'make it right, then make it fast' (not to try it the other way around).
There is a huge amount of advice regarding best practices, for example at: javapractices.com. We can't expect that submitted code is checked against all best practices. Here, for emphasis, are what we regard as the most important issues:
Length of code
Probably the most important issues to look at relate to code length:
- class size - look at the largest class: could it usefully be split into several smaller classes?
- method length - look out for method definitions consisting of many lines of code. It is likely that refactoring of these into smaller methods could improve the ability of someone new understanding what the code is doing.
Another issue can be package structure. Each class should be organized into a comprehensible package structure. There shouldn't be too many packages, nor too few. As a rough guide, if a package contains less than 4 or more than 10 classes, it may be worth rethinking.
Another very important issue which is often under-rated is naming. Are packages, classes, methods and variables in your view, optimally named?
- do you understand all the names?
- do the class and method names express their purposes well?
As a code reviewer, you might fear that you are being "picky" by complaining about names, but naming is a critical factor in the maintainability of code. If names are well-chosen, they serve as abbreviations of the functionality they name, which assists enormously in comprehending the whole. If a method is not well-named, one has to read each line of code to understand what it does, which is a very inefficient alternative.
- Is the person submitting the code on the list of people who have confirmed transferral of copyright of code submitted?
- Am I having difficulty in understanding this code?
- Is there any complexity in the code which could be reduced by refactoring?
- Is the code well organized in a package structure which makes sense?
- Are the class names intuitive and is it obvious what they do?
- Are there any classes which are notably large?
- Are there any particularly long methods?
- Do all the method names seem clear and intuitive?
- Is the code [well-documented](http://www.javapractices.com/topic/TopicAction.do?Id=60)?
- Does it follow common Java code conventions?
- Does the code submitted introduce unnecessary dependencies which reduces the modularity of the codebase?
- Are there ways in which this code could be made more efficient?
- Are exceptions simply swallowed anywhere, rather than being handled explicitly?
Normally we deploy only release branch, which should contain only validated and tested (all tests pass, manual regression tests pass) version from master branch.
Deploy only using OPS Code tools (provisioning) and buildbox environment.
What is an Icebox?
Icebox is a place in our project management software where everybody can propose a task. It contains stories that have not yet been prioritized.
On most of our projects, we find ourselves discovering new stories all the time, based on the continuous feedback loop that Tracker encourages. Not all stories end up in the backlog, but it’s liberating to be able to capture new ideas and user feedback as they occur.
The downside of this is that the icebox, where all new and unprioritized stories live, can grow out of control very quickly. It’s not uncommon for longer lived, active projects to end up with hundreds, even thousands of stories on ice.
More information about it and related process you can find in the Defrosting Your Icebox article.
Synchronizing: merge or rebase?
Read a great merge or rebase post by experienced people from Atlassian (the creators of bitbucket and tools for git and mercurial management). Summarizing:
- Rebase by git pull --rebase when syncing shared feature branch (with others change on that branch). You can make it default for pull by: git config branch.autosetuprebase always, but then remember to not use pull for different branches (you can use pull --no-rebase or simple fetch + merge)
- Merge when syncing with other branch. Don't use rebase for this (especially when you share the work)!
- Linus on git rebase: git pull
When should I push my commits? What if I need to break in the middle?
Push when you finished your feature/mission or when you finished logical part of a task. Normally you shouldn't push every commit update, unless you are working with someone. Having lot of checkpoint might be good, but push only the good one (others just squash).
When you need to switch machine, or stop in some unfinished shitty place and you are not certainly when/where you continue then you can always make a side branch feature-x-shit-y (treat it as a private branch) and push it. When you come back to continue when you can squash fixes and middle-in-the-shit checkpoints and rebase on the head of feature branch. But it should be really rarely - because the missions should be short.
It is an easy recipe for efficient workflow, where we can efficiently debug the history.
When can I commit directly to master?
You can commit directly to master when your change:
- is small (eg fix, readme update, logging) which fits in single commit, and
- doesn't change or bring new logic.
- you are sure you don't need backup.
- don't share the work with someone else.
Usually those cases are rare. Often you want to review your code and create a new branch.
If your change is small and fits in one commit than you can cherry-pick it from feature branch instead of merging (read the last, mentioned in previous question tip )
Are you afraid that Git lost your commits?
That is not the case. git preserve the branch you were on until the rebase successfully completes. In the meantime you can always do a "git rebase --abort" to undo the rebase. After it commits, you can use git reflog to see all of the commits that were supposedly "lost" or "uncommitted" from any recent rebase (since the last garbage collection which for me is less than once a month). You can easily create a branch for any of those commits using "git branch foo " and look at it, switch to it, etc. I have NEVER lost a single commit doing a rebase after many years of using git heavily. When there are no conflicts (usually the case) then everything happens totally smoothly. When there is a conflict, either I easily resolve it or else I "git rebase --abort" and react to the kind of conflict I got such as reordering or squashing the commits using (git rebase --interactive). If several of my commits conflict, I may use "git merge" so that I only have to resolve conflicts once, in which case the noise in the history is worth it.
What is our branch naming convention?
- [fix] - optional part when update is a bugfix
- number-17 - pretty task which touch all components (server, client)
- webclient-quoteNew-select2 - select2 implementation in quoteNew component in client
What to do when you can't find settlement with reviewer?
Call him or find other developers who will make a quorum.
How to make review issues
While commenting / discussing a pull request we need to track what needs to be done to accept the pull request. Asana is our tool to track all tasks so it reasonable to use it for review issues (issues from particular review) tracking.
By adding a subtask to review task:
- developer should set a high priority for this task (unless he is in the middle of other important thing and don't want to be disturbed).
- we can see what lefts
- we can see why the main task is not complete yet
- it helps reviewer to accept the pull request (by tracking what lefts)
- it helps feature author to finish the pull request by prioritizing his tasks
Good habit is to put a link to a commit/comment page related to this issue.
What are non-blocking review comments?
- invades on some decision which is questionable to achieve or
- comments are not constructive
then such reviews are non blocking to approve the pull request. Force reviewer to add constructive comment. Otherwise when it doesn't help ask someone else to approve such commit.
All pushed code should confirm to the code style:
- Default max code line length is 96. To visualize it you can use
- In Emacs: fill-mode, or FillColumnIndicator plugin.
- gofmt and revive for .go files.
- coffeelint for coffeescript files. All config is default, except max_line_length which is 96.
- pylint for .py files.
- You can configure your editor to use gofmt and coffeelint for on-write-hook.
- High-level code should be self explained.
To meet all these requirements you should:
- properly configure of your favourite editor
- load programming languages plug-ins for your favourite editor (they should take care about automatic indentation)
- activate git hooks. More info about them in project INSTALL.sh file
Choose good names for the code:
Code comments & docs style
Code is our common good.
- All the style is verified by lint tools (yes it works also for comments).
- Comments should be compact and readable.
- TODO, FIXME sections should be linked with corresponding tasks on Task Management Tool.
- Good code brings better info than any comments.
- Prefer good functions, variables names then explaining cryptic things. Of course you can use short variables.
- If you need to explain most part of the function, then usually the function is wrong constructed. Try to refactor.
- If there is already a place for a comment domain, then don't repeat that part in other places (eg: React js component arguments should be documented in propsType rather than @props).
- If you need to comment function arguments in Go then don't double info about argument type. It is already there (in a function declaration)! Proposed syntax: @arg-name description
// LoadConfig opens and parses a yaml application config.
// @path principle path to read a config. If it's an empty string,
// or it points to not existing place then it will try to read
// config from "/etc/.../config.yaml"
// @return error if can't read or find a config file.
func LoadConfig(path string) (config *yaml.File, err error)
What is a filename convention?
For .go and .py files:
- we use underscore to separate_words_in_filename
- test files have a _test suffix
What is about code generation?
Some code is generated by separate program - for features which are better resolved by a machine then a human. In particular this goes to:
- grammar parsers
- enum printers (go stringer)
- documentation generators
Whenever touching develop branch make sure that code generators will not introduce new changes (all potential changes should be committed). All generators should have tests to check if everything is committed.
When changelog should be updated?
Changelog (web/changelog.md) presents a broad outline of features changed / introduced in application. Whenever you make a new feature or make a significant feature modification - which will change a use case then you should include it in changelog before doing PR. We are not going to list all things there - these we have in commit log.
- Gitlab handbook