Agree on a coding standard or agree not to

Posted in September 2012 by under developer

I have worked on large projects where the coding standard was very well adhered to and yet the code was a tangled mess. I have seen well designed applications where the code was just written out without any thought for coding standards in a mixture of styles. Although I prefer to use a coding standard, I am not saying you should. Do whatever you like, be happy.

However I think when it comes to working on collaborative projects, it is reasonable to do one of the following

1. Agree to use a coding standard, pick one and stick to it. Set up everyone's IDE to apply that coding standard. Get your build servers to run codesniffer and check for violations. Refuse pull requests that deviate from the coding standard.

2. Agree not to use a coding standard. Make sure new changes fit into the surrounding code and DO NOT REFORMAT EXISTING CODE.

The reason for this is pull requests. Say your developers are working on feature and bug branches. Once the changes are complete, a pull request is made so another developer can peer review the changes. They can check it merges, is consistent with the direction of the project and fixes the issue it is supposed to fix. Once confident they aren't going to knock the staging server over, they merge the changes in.

Life is good. Work gets done. Everyone is happy. Until you get the pull request that has only 17 lines of actual changes but the diff is 1818 lines long. You are going to have to play a lot of spot the difference to determine what has actually been changed and what is just someone moving stuff about.

This is not what pull requests should be about. This is just a waste of time.

The real solution here is to refuse the pull request as 1801 lines of changes are not related to the issue or bug. If the code was really so bad that it needed tidying up, then that should have been it's own issue.

If you had chosen option 1, at this point you could take the branch with the changes and automatically reformat the code using your configured IDE. This effectively wipes out the noise and you can then see the actual changes. You would not be able to do this if there was no defined coding standard set.

If you had chosen option 2, you could simply reject the pull request as 1801 lines are not fixing the issue and should not have be changed.

Either of these approaches seems preferable to accepting 1818 lines of changes for a 17 line fix. I am assuming everyone agrees at this point.

The problem is this.

If you haven't explicitly agreed on either 1 or 2, it becomes difficult to explain to the developer why the pull request is going to have to be rejected.

They will argue the changes work, are tested and have taken two days to do. The alternative to accepting the pull request is for them to do the work again and loose two days work. In this situation things will almost certainly result in a pointless argument about the use of whitespace in parenthesis, tabs or spaces for alignment and variable naming conventions.

You are then in a position where it actually makes more sense to accept 1818 lines of changes for a 17 line fix than it does to argue the toss. This is madness that can be avoided if you agree to either use a coding standard or agree not to and leave existing code alone.