All code should have a second set of eyes on it at some point. This serves the following purposes:
1. Catch bugs or potential bugs.
2. Teach both the reviewer and reviewee.
3. Keeps the team members up-to-date with what other teams members are working on.
4. Introduces best practices, new features/techniques and new libraries.
PRs should almost always be assigned to someone. Others can be @mentioned. Almost anyone (senior enough) should be able to signed off with a Looks Good To Me (LGTM) in the comments and merge.
The developer submitting the PR should let the reviewer know the urgency of the PR.
1. Urgent. The submitter immediately accepts the PR. The review is done post-facto as a learning and review process. Can also catch bugs that are fixed in the next PR.
2. Normal: Someone is expected to review this within an hour.
3. Low: Review within the next day.
An automated linting tool should be part of the commit process. Locking in lint errors to a maximum value will allow the gradual introduction of linting to existing projects.
Developers should be encouraged to have a lint helper plugin in their editors so they rarely/never see lint errors on commit.
The team should have an evolving .eslintrc file that's shared across all projects. Some projects might have specific override .eslintrc rules if they are catching up with the standard or have exceptions.
Keep the master .eslintrc in a dot-files folder that's a sibling of all the other projects and reference it from there.
ES6 is here. There are very few reasons not to start using this. Babel is an excellent transpiler that will allow developers to start using those features.
Use code reviews to help teach other team members ES6
If you're not using ES6 you'll start leaking developers to other teams and companies. More on this below...
Most projects should have tooling setup so that they can quickly iterate on small changes to code that they are working on and see the results.
This rapidly accelerates both learning and development.
If, for example, you're working on a React/Node project then, in my opinion, you'd have the Webpack Hot Module Replacement
transpiling your changes in memory and making them almost instantly available in the browser.
As developers we're always on a learning curve. We can never be expected to know everything. Using a fast responsive hot loader we can quickly try different techniques and see the results. This will speed our learning of how that corner of the world works.
Front End Framework
Most projects should be using a modern front-end framework for the UI component. The most popular over the last several years has been AngularJS. The current rising start is ReactJS.
I'm in favor of ReactJS
. It looks like AngularJS and EmberJS are converging on what ReactJS has introduced.
We should strive, as much as possible, to be using the latest versions of everything. It's perfectly acceptable to hold off on the brightest and shiniest until it's stable. That should be a conscious decision and not the default.
Npm has a great command npm outdated --depth 0
that will give us a summary of how our project looks compared to what's available. Use that command with the -g
option to check your globally installed tooling.
The 3 main reasons usually given for keeping up with the latest version (1. Features 2. Performance 3. Bug Fixes) are completely irrelevant. The reason you want to stay current is to keep your developers happy and engaged. Nobody wants to work on yesterday's technology.
Keeping a buzz in the team about moving to the next version will keep everyone up-to-date on what's happening and engaged with the team. The grass-is-greener syndrome with other teams or companies won't exist on the technology front because you'll be using the latest. In fact I've successfully used this as a recruiting tool for developers who are stuck in other companies or teams on old technology.
Your tooling should automatically keep your project's dependencies up-to-date with what would be installed when someone initially installs the dependencies on the project. I use the npm-update-outdated
module as part of my pre-commit hook to update my dependencies before the tests are run so I know that my tests have run against what the CI server will use.
If someone asks you to work on a project then you should be able to do the following pretty quickly.
1. git clone
2. npm install
2. npm test (should run clean)
4. npm start
And have a server up and running that the browser can hit and interact with.
There are lots of complexities that might stop something that simple from working.
The project's readme should have bullet proof (as good as possible) instructions on how to address that.
For example, most developers have Docker. If you have a dependency on a MongoDB then npm start or npm install should run Docker and pull down a MongoDB container and get it up-and-running.
A new-to-the-project developer who experiences problems with an out-of-date README.md and
who then updates the README.md should be given the rest of the day off as a reward. (That might be a bit extreme. This almost never happens so it might also be okay.)
All projects should have tests. These should run during a pre-commit hook. Ideally these are run implicitly through the Code Coverage tool.
When bugs are fixed a test should usually be written demonstrating the bug before it's fixed. This test will prevent a regression on this bug later.
Tests are difficult to write. Do lots of cross-education with other team members during PRs with tests. Get to know and love all the tooling around tests. When you're using a tool for the first time ask for help and opinion in the PR.
Some of the tools I use for testing:
Code coverage such as Istanbul can be automated as part of a pre-commit hook. It will help make sure that critical sections or sections that you're about to change have sanity tests. It has awesome "graphics"
Code Coverage Minimum Levels
It's easy for a developer to commit new code that doesn't have test coverage. In my enthusiasm to get my new code out there I often forget to write tests for it. Using a tool like Istanbul we can prevent code coverage from dropping from the previous level in a pre-commit hook. This will remind us that we need to add tests.
Least of all you should never have duplicate code that's duplicated for the sake of using on both server and client. There is almost always a way to provide the same code block to both client and server to provide a DRY and isomorphic code-base.