2

Long story short: An older new(ish) starter with more industry experience is blindly ignoring our coding standards. I don't know the best way to resolve this.

Sorry for the long post, thanks for reading!

A new(ish, only a couple of months in) developer at my company who is 10 years my senior in age and industry was tasked with a new project from his start date. This was meant to be almost exclusively his project with help here and there from other members of the team. So for the most part I've not been looking at this partly due to the fact the higher ups wanted it to be a pretty much solo project, (a fresh start), and partly due to other commitments.

I am the most senior developer on the team right now and have been here around 3.5 years, and before he started I shared with him our teams coding standards document. I noted we could discuss any issues with this and look at changing potentially anything if it was in agreement.

He did not start this new project straight away, but was given a couple of small tasks to get used to our environment. I immediately noticed small, but numerous bits of code that we're not following our standards and I chatted with him about this to see if he could be more careful about following it. To which I got a seemingly positive response somewhere along the lines of, "sorry, yes of course. I just need to get used to the change". All sounded fine.

A couple of weeks later when I had a brief meeting to go over his work on the new project I noticed the same issues, and once again mentioned it to him. This time I said we could have a different standard for this one project if he could write up a document explaining it. However I got the same response as the first instance, and he said he would change the code to match our standard.

Now a month or two on we've had one other developer working with him doing the front end work, and I'm now noticing problems occurring. This other developer has no idea how the code works. The fact that it is not following our standards is starting to have a negative effect on our productivity.

The new project has been built on top of a previous application this new developer had built in the past. We all knew this from the start and have accepted there are going to be a lot of differences to what we're used to. This project is requiring a quick turn around, so this decision was accepted. However some decisions have been made by this new developer that are either completely absent from the coding standards or once again the opposite of the coding standards.

This is really starting to get on my nerves at this stage. I look at his code, and my heart starts to sink and in my head I'm saying to myself "why the hell did he do it this way!" or "why didn't he do it the way we've always done it! What's the point of our standards if they just get ignored!"

I was mid way through writing an email to discuss these things with him (currently on vacation), until I realised I may word my frustration incorrectly and cause other problems as a result.

As this developer is a fair bit older and experience, I can certainly understand he may know better ways to do things, but the things I'm picking up on do not fall into this category, and in one case I know for a fact his change is worse (security wise) than the way we've done it in the past.

I'm not sure of the best way to get my message across to him that he needs to stop doing everything his way. I would never go to a new job and just write every bit of code exactly how I do now. I would do it the way the company are used to doing it, and suggest alternatives if I had any real issues with it. This guy is not doing that. I feel like at every point, he's decided to do things differently and has completely ignored our coding standards.

I'm in a more senior position to this developer in the hierarchy, however I'm not his manager, so I don't feel comfortable telling him directly what to do. His actual boss and mine, is not really the type of person to enforce something like this either as he's not a programmer, he wouldn't know where to start, so I'm not sure if that's a solution.

What's the best way to resolve this?

It's worth noting, I have no personal issue with the developer, I get along with him just fine and have worked on tid bits here and there without any issue.

David K
  • 30,066
  • 21
  • 108
  • 140
not a manager type
  • 31
  • 1
  • 3
  • Have you come to these code standards together as a group, with everyone buying in? Even new coders will have trouble accepting stuff like this "just because". – Garrison Neely Aug 17 '16 at 16:30
  • 9
    Any reason you cant use something like StyleCop (or equivalent for your language) in your CI pipeline to reject incorrectly styled code as a failed build? You can even set it up on a commit hook for your source control, so it doesnt even make it into the repo... –  Aug 17 '16 at 16:36
  • 6
    Your manager shouldn't need to be a programmer to help reinforce the need to align to company standards. I don't know accounting, but if I'm managing accountants and a Sr. Accountant tells me newly hired Accountant isn't following the company's defined standards, I'm going to have a conversation with the newbie to reiterate the necessity. – Chris G Aug 17 '16 at 16:36
  • 3
    Are the standards in question simple formatting issues or design issues? – Patricia Shanahan Aug 17 '16 at 16:54
  • No programming lead is a problem. I think you need to go to your manager and explain the problem. If he knows how to apply the coding standard then why not just do it in the first place? If he disagrees with the coding standard then he should propose changes. – paparazzo Aug 17 '16 at 17:01
  • Note: Any competent programmer should be able to write in, and read, any of the common stylistic conventions for the programming language they are working in, and should know to comment anything which isn't obvious. So, yes, the newbie should be adapting to your conventions, but it shouldn't be a big deal when they don't. At worst, it just means you run their code through a formatter and check it back in, right? – keshlam Aug 17 '16 at 17:19
  • If your coding standards are as long as this question, I don't know if I'd read them either ;) –  Aug 17 '16 at 17:25
  • I would still talk to your direct boss about it, don't assume he won't / can't do anything about the break in standards. – RandomUs1r Aug 17 '16 at 17:45
  • These standards have been agreed on by all the previous devs. We're a small in house web dev team of 4, not an agency. Most of these standards are indeed simple formatting issues. Our standards have been based on well documented standards written by established organisations and tweaked where we see fit. The coding standards are long, but they have to be to cover all aspects. We're covering 4 languages within it. some images and code snippets buff it out too. We currently do not have any automated testing for anything like this, so the code reviews have to be manual. – not a manager type Aug 18 '16 at 08:29
  • @notamanagertype, how long are the coding standards? E.g. if printed single-sided on ordinary 8.5x11 or A4 paper, how many pages would they be? –  Oct 17 '18 at 08:27
  • Peer review of his code. Reject incorrectly formatted code. Repeat as needed. Ensure IDE formatter can format according to your code standards! – Thorbjørn Ravn Andersen Mar 25 '19 at 14:06

5 Answers5

14

If you are using a properly defined process for build management, including source control, continuous integration and automared unit testing, then you can handle this quite easily - treat incorrectly styled code as exactly what it is, a bug.

In the .Net world you would use StyleCop (or an equivalent) which either takes the code before it is committed to your source control (a pre-commit hook), or it sits in your CI pipeline, and it examines the code for inconsistencies between the code and the defined style the code must conform to. If the code does not conform to the style, then it is rejected and the build (if you are using it in the CI pipeline) is marked as failed.

Extra credit if the failure triggers blame emails to all other developers....

  • 8
    Extra credit if the failure triggers blame emails to all other developers.... I do not think that is particularly helpful – IDrinkandIKnowThings Aug 17 '16 at 16:52
  • 3
    @Chad its incredibly helpful, its a very public notification of build issues done in order to solve exactly these issues. You should never break the build, which means bad code should never be committed to the repository - including styling issues if they are considered not acceptable. Dev teams use many different systems for this team-based character building exercise, including blame emails, klaxons on failed builds, electronic notice boards with build status that flash red etc etc etc. It solves these problems very quickly. –  Aug 17 '16 at 16:56
  • 9
    Public shaming on breaking a CI build (which has at its entire purpose finding failures) is a pretty bad idea. Particularly if you are adding a new functionality to the CI system itself. – enderland Aug 17 '16 at 18:18
  • @enderland uh, CI is there as your last resort and test-in-depth solution, as well as packager and other things, not as your general build-breaking bug catcher - you should have built the code locally and run the relevant unit tests (if not the entire suite, especially if the suite is extensive and long running) and only then committed to the remote repo. So no, "shaming" on a broken build is not a bad thing, especially for stuff like this - a breaking build is different to a failing build, a failing build may fail unit tests and that is what CI is there for, not to catch breaking builds... –  Aug 17 '16 at 18:25
  • 1
    I have worked in many teams where such emails would end up causing real problems. It might work on your team but most the of the teams I have worked on would have caused far more problems than they avoid. – IDrinkandIKnowThings Aug 17 '16 at 20:29
  • I'm not aware of automated tools like these for our current languages, however I'm looking into that right now, but I think this certainly sounds like a good move. – not a manager type Aug 18 '16 at 08:31
  • @notamanagertype what languages are you using? –  Aug 18 '16 at 08:58
  • @Moo general web dev languages. PHP, JavaScript, SCSS, HTML – not a manager type Aug 18 '16 at 13:53
  • @notamanagertype oh, there are some very good tools for those sorts of things - for example JSHint for the Javascript, PHP_CodeSniffer for analysing PHP, CSSLint for the CSS, html5-lint for the HTML etc etc etc. Tie it all together using a CI tool such as TeamCity to check out the repo and run the various tools, and enforce it by using something like Octopus Deploy or Puppet to deploy a package which only a successful CI build creates. Get rid of manual website deployments, those propagate many issues such as the ones you are running into. Force devs down the CI+Deploy pipeline. –  Aug 18 '16 at 14:05
  • Some source control engines - actually all the major ones - have a pre-commit hook you can execute that can do anything you want. Including checking coding standards and rejecting them or sending out a notice. – Dan Aug 18 '16 at 19:18
  • 2
    I think sending out a "blame" notice is generally a bad idea. It should just reject the changes without any public notice. If the person can't correct his code to the format needed, then his work would be late and he'd have to come up with a better excuse than, "the source control engine rejected my changes." – Dan Aug 18 '16 at 19:29
  • I will look into the whole CI tool thing properly. Had a brief look at PHPCI, but havent had enough time to really look into its configuration, but it looks roughly like something that will make a difference. Thanks. – not a manager type Aug 19 '16 at 15:48
  • Tools > Documented standards. We have as part of our TFS gated checkin (c# world) a validation step that runs the code through the resharper rulebase (which is a combination of stylecop and FXcop rules). If the code doesn't comply, check in fails. – Jon Barker Oct 15 '18 at 13:45
2

There are a couple things your team can do to fix issues such as this.

  1. Add code review as a step in your process for all your projects, and ensure that specific sets of developers/testers/managers are included as reviewers on the pull request. When he submits a pull request that fails to meet coding standards, comment on it and either decline or mark it as "needs work". This works both ways - all your codebases will improve in quality.
  2. You can create a ticket on whatever issue system you use to address the issues, and assign it to him. The ticket would not be closed until your team agrees that it's up to snuff
  3. Schedule a meeting with him specifically designed to discuss these issues. You should both walk away from the meeting with action items such as the above mentioned ticket being prioritized, or a managerial review of coding standards if he brings up excellent reasons for doing things his way

If all fails and he just keeps ignoring you without even trying to address the issue, bring it up with your manager during an appropriate 1 on 1 meeting. If he manages both of you then he can help get to a resolution directly, or can reach out to your coworker's manager to help figure out the situation.

Derek
  • 155
  • 1
  • 5
2

Do regular code reviews with the entire team, either daily or weekly, depending on what's right for your group. Correct any code it before it merges to the main (master?) branch of your repo. It's likely you're just having issues with a dev who can't do the work to spec without plummeting in productivity. Code reviews will help change that, and force all code through a filter that correct errors.

jimm101
  • 12,000
  • 7
  • 38
  • 63
2

There are two possibilities, and you have to figure out which one it is. Either you have coding standards that are just there to have a rigid standard, and don't actually add any value. Or worse, are contradicting industry standards. And this developer ignores your standards because they don't contribute anything of value. That would be your problem.

Or you have a developer who is so rigid in his ways that he cannot adapt, and that's his problem. (And your problem as well unfortunately). Find out which one it is.

Personally, I'm old enough that I don't care what standards you have as long as you don't have more than one set of standards :-) Most of the time it's like driving on the left or the right side of the street: Each is equally fine, but you better agree with the other drivers. If there are things in your standards that are badly counter productive, I'll tell you, and it's up to you to change it or not. Other than that, code should follow one standard so that's what I do.

It should be enough to ask him politely to adapt his style to the common standard. And everything after that depends on how he reacts to being asked politely. Obviously you should code reviews, and a code review for a week worth of changes that just says "could you please change this to our coding standards and then I review it again" will probably work wonders.

PS. Anyone who calls a style checking tool "StyleCop" seems to have a very bad attitude to people doing their job.

gnasher729
  • 169,032
  • 78
  • 316
  • 508
  • 4
    StyleCop is the official name of actual tool created and published by Microsoft (now under the Micorsoft Public License) for checking code style and consistency. It's not a cheeky name for a generic process. https://stylecop.codeplex.com/ – alroc Aug 17 '16 at 18:41
  • 1
    I ddn't say it was cheeky. I never thought it was cheeky. I said it shows a bad attitude to people. I'm not surprised something like that would come from Microsoft. Process over people. – gnasher729 Aug 17 '16 at 20:48
  • 1
    Could be a traffic cop rather than an arresting officer, so I think objections the name say more about the objector than about the name.... And I can't believe Im defending a Microsoft product. – keshlam Aug 18 '16 at 00:05
  • I'm sure there are some standards we have that are a bit over zealous. I am perfectly willing to change these though. I believe this dev is also willing to change, but he is potentially as you suggest in your second para, to rigid in his ways. – not a manager type Aug 18 '16 at 09:47
  • @keshlam: Either way, if a cop looks at something you've been doing then the assumption is that you are doing something badly wrong. That society (or employer) needs to be protected from you. Someone was given a search warrant for your code. – gnasher729 Aug 18 '16 at 14:20
  • 1
    @gnasher, If that's the association you have with the word, fine; it isn't mine and gods only know what's typical where or for who. Your milage may vary; void where prohibited. – keshlam Aug 18 '16 at 14:52
1

There is a common system called buddy check, which is a form of peer review. On a check-in, another dev - i.e. a buddy - looks at the code and points out small fixes to be done before the check-in*. If you do check-ins every 15 minutes, do the buddy check on every merge. There should be no more than a handful of buddy checks per developer per day.

The buddy must be stated on the check-in comment, which can be enforced with tools. If the person who programmed the code is not available, the buddy is responsible for any questions or issues regarding the checked-in code.

Hold the buddy responsible for code that doesn't follow the standard. If code that violates the standard still gets submitted, then 2 people think the submitted code warrants breaking the coding standard, in which is a sign that the coding standard may need improvement (e.g. it might be too strict).

*In reality, the dev who explains their code to the buddy is usually the one who finds and fixes more issues during the buddy check.

Peter
  • 14,539
  • 2
  • 30
  • 52