8

I'm developer infrastructure and tools engineer.

My org structure is like so (fictional names for convenience)(job level in parenthesis)

Bob(3)/
    Me(1)/
    Alice(2)/
       Sam(1)/
       John(1)/
       James(1)/
       Joe(1)/

Alice and Bob are managers. I report to Bob as does Alice, but I might as well report to Alice as I'm directly answerable to her in terms of deliverables.

Our team works in sprints. Last sprint a senior dev at Bob's level remarked to Bob that the code quality in some of our codebases could be better. We don't measure code quality as a tangible metric, but just code review from senior devs.

So Bob asked me to do something to improve it as I'm in charge of the dev infrastructure we use eg: git, code review, computing resources, CI/CD etc.

The particular complaints raised had a lot to do with errors that could be fixed with linting and checkstyle, and we have had errors that proper linting would have prevented, so I decided to attack low hanging fruits first. I went ahead and implemented mandatory linting and checkstyle as part of the build process, but I did not condition the builds to fail but rather warn.

But the code quality did not improve so the next sprint Bob asked me to do something a bit more seriously and made it one of my deliverables for the quarter.

Bob is not an ex dev and not familiar with finer details of the dev process but is still reasonable.

So I sent a doc to Alice and her team illustrating why they should lint and then I went ahead and conditioned builds to fail if the linter and checkstyles did not pass for new code, and my mailbox exploded with email asking me to disable the linting and Alice let Bob know that my actions had adversely affected their team's ability to meet the sprint goals.

So Bob asked me to disable the linter.

My question finally is how do I go about convincing them to fix the code quality without alienating my managers or my developer friends? I've tried educating them by sharing documents with good coding practices etc. but they don't even read my email with subjects like that.

Also how should I communicate to Bob that disabling linting will hurt our project in the long run without throwing Alice and her team under the bus? (There could be a situation where I report to Alice tomorrow or have to work closely with her team)

If Anyone is curious how this ended

  1. Bob gave me the power to add a limited number of sprint goals for Alice't team.
  2. I proceeded to stagger the fix for the code style warnings among the devs in alice's team and myself
  3. Every sprint the last few tickets addressed are code style fixes
  4. Our warnings have steadily gone down :)
  • 14
    Did you talk to the team about quality before enabling linting or did you just switch it on? – tddmonkey Aug 7 '18 at 6:08
  • 1
    Comments are not for extended discussion; this conversation has been moved to chat. Please keep any discussion on the (de)merits of linting or other coding best practices contained to the chatroom. – Lilienthal Aug 9 '18 at 14:55
27

My question finally is how do I go about convincing them to fix the code quality without alienating my managers or my developer friends?

Since Bob is concerned about code quality, and Alice is concerned with meeting sprint goals, the obvious choice is to define a sprint goal which involves improving code quality.

Then the team can integrate that goal into whichever Sprint they deem appropriate when weighed against all their other goals.

Make sure you have a solid definition of "code quality" so that the team can understand it before they have to size that goal and create tasks to meet it. You might also want to seek agreement with Bob and Alice on what "code quality" means in your shop beforehand. Finally you need to have an agreed-upon way to measure code quality, so you'll have some way to objectively determine if it has improved or not.

  • @Chan-Ho Suh Assuming we are talking about JSLint, it must be said that it is a strongly opinionated tool. Even its own developers describe it as a new language which is a subset of JS. Moving to this, even if fully agreed with the team, would 100% require (most likely severa) sprints to get the code in shape. It cannot be done overnight. – Joe Stevens Aug 7 '18 at 6:59
  • @JoeStevens agreed JavaScript is its own beast and ecosystem; we don't know the OP is talking about JS, but if s/he is, then there could be a number of issues with linters. None of that was clarified by the OP, but in general, in 2018, languages that have linting standards have them for a reason, and mostly they work fine. – Chan-Ho Suh Aug 8 '18 at 3:17
11

I'd say the way you handled this was very inappropriate.

Your code apparently produces lots of warnings. If you prevent any other work from being done until these warnings are all gone, then you block everyone from doing their usual work. No wonder you got complaints from everyone, including Alice. This is a totally unproductive way to handle this because suddenly everyone is blocked.

If reducing the number of warnings is important, then first you make sure that producing these warnings is in everyone's build process during development, and then you pick a file with say 100 warnings and put a task into your sprint "remove all warnings produced by file X", and then another task for the next file and so on. So anyone can pick one of these tasks, and the quality improves bit by bit.

The way you did this, you created a huge amount of work for the developers, which has priority over everything else (because nothing works until everything is free of warnings), which blocks everyone but cannot be shared (because if you have five developers, they can't each fix one fifth of the warnings), and is not recorded anywhere (so the devs look as if they just put their feet up for the time until all warnings are fixed). No wonder they are up in arms and Alice overruled you.

  • OP did say only for new code, and it is a standard feature of linters that they will excuse existing code but still check changed lines. Still, introducing a new gate of any type in dev process works a lot more smoothly under consensus of how it will work (and time to adjust estimates etc) – Neil Slater Aug 9 '18 at 15:48
  • 1
    @NeilSlater Even if it was only for new code, if the use of linters/etc. wasn't discussed properly before hand (and tddmonkey's comment to the original question asking this seems to have gone unanswered) then it's still the wrong way of doing things. – TripeHound Aug 10 '18 at 15:01
11

I've actually done this at some previous company. We had a project that produced about 2,000 warnings. Hidden within these 2,000 warnings were a few dozen bugs. Together with another developer, we started reducing the warnings; often hundred warnings with an hours work. When the number of warnings went down to 500 we turned on more useful warnings. Eventually we were down to zero warnings, and then we turned on a setting "warnings=errors" in the build system.

That's how it is done. If our boss had turned on this switch at the beginning, we would have had weeks with several developers doing no useful work.

  • 1
    Pretty much only way. Rule by rule (or block of rules and block of rules). Otherwise you don't see anything in the mountain of warnings. +1. – TomTom Aug 7 '18 at 17:34
  • @TomTom: Alternate rule: the build fails if the number of warnings increases. – Joshua Jan 8 at 23:15
7

First and foremost you need to talk with your team and address the problem with them and work with them on what can be done to solve the issue. If you want to solve a problem with a developer team the best method available to to do that is get them involved with the process themselves. They may have to make changes that they don't want to but if they are involved they can better understand the process and bring up issues that you might be missing (such as pressure to deliver code faster regardless of quality).

Second you might also want to remember that management that is not as familiar with the development side might get to caught up in buzz words and not fully understand that what they want isn't what they already have.

5

This situation reads very passive-aggressive. My question to you is: Did you get Alice's input before you turned on CI/CD failure for the linter? Did you consult any of the developers? Heck, did you even consult Bob? The way this reads is "I decided one day to flip a switch which caused everything to break, without explaining to anyone what I did, why I did it, or why it was important", which is very passive-aggressive and destructive behaviour. If this is how it went down, I can understand why Alice and Bob are both upset with you.

Here's what I would do: Firstly, document how many issues there are with the code, based on how many CI/CD warnings you get with the linter turned on to warning level. Then you can show Bob and Alice that the code base is crap and something needs to be done. Then Bob and Alice can prioritize how important it is to fix these issues over getting sprint deliverables done. Then, if they decide that delivering results is more important than keeping clean code, you have an out: just say to Bob "I can't do this because I'm not a developer; tell Alice to get her team in gear to fix these issues" and drop a pile of documentation on his desk. Conversely, if Alice and Bob decide, jointly, to make finding and fixing issues a priority, then you can provide a list of issues to Alice to have her team prioritize and fix; once the bugs are deemed fixed (you can check this in your CI/CD dashboard), you can turn on the linter to maximum to make sure the issues don't reappear.

3

From a purely technical viewpoint, lining and style cheeking are two widely different beasts and have different goals (code quality & legibility/maintainability).

If you have not previously linted, then the first time that you turn it on, you are going to have a massive – but one-time – hit. That will have to be planned as effort in to as sprint. After you have lint-error free code, it is easy to keep on top of it.

As for style cheeking, you can use a code beautifier (if in doubt, ask here) to your build process. Perhaps even the threat of one will be enough to enforce voluntary adherence to guideline. Doing so manually will mean a further one-time effort hit, which also has to be planned into a sprint. Be aware that doing so may give problems with file compare pre/post-beautification. Hopefully, moist differences will be whitespace an can be ignored, but not all (for instance, suddenly dis-allowing multiple return statements form a function).

As others have stated, you cannot expect to introduce these with no warning and expect everything to go smoothly – especially if you do not allocated sprint time. It is also important to get the team to “buy in” to these practises, which means explaining how they will benefit (feel free to ask another question about that ;-).

After you get them on board and the first major effort behind you, it is up to you whether to treat infractions as warnings or build breaking errors. A compromise might be to say that more than X infractions breaks the build, whereas less simply emails the malefactors.


Just curios: which language? And do you have automated regression tests in place?

  • 1
    I'm curious why you think using a beautifier is a threat? I consider it a requirement when you want to enforce a certain style. – Erik Aug 7 '18 at 9:24
  • I personally don't see it as a threat, but as a help, like you(+1). But I have seen plenty of others who oppose it - either because it was a poor tool or was poorly configured, and generally for the same reason that they dislike (certain) code style guidelines (tabs/spaces? indentation level? braces on the same/next line). – Mawg Aug 7 '18 at 9:55
  • 2
    Beauty is in the eye of the beholder. Just because it's called "beautifier" doesn't mean it doesn't mess up my code. There are some real crap tools out there. – gnasher729 Aug 7 '18 at 10:55
  • I totally agree - on both points. 1) call them code formatters, not code beautifiers 2) get one that can enforce your (project's) guidelines - ask heer if you need one. They can be useful - if you find the correct one & know how to tweak it – Mawg Aug 7 '18 at 11:42
  • 1
    A beautifier is definitely a threat when you introduce it. As Mawg said it makes for massive problems when comparing code from before and after the implementation. Once it is implemented, and as long as there is a well-agreed standard that is properly programmed in the tool, then it should be fine. I've implemented things like this and find that it works best as the first deliverable of a new release. You are much less likely to compare release vs release than in-release. – Sinc Apr 26 at 18:31
3

In short: make a webpage that shows the lint result for each build, and has a build/lint result graph. This lets Bob see progress, and makes everyone else aware of the problem.

You were asked to improve the code quality:

Our team works in sprints. Last Sprint a senior dev at Bob's level remarked to Bob that the code quality in some of our codebases could be better.

So Bob asked me to do something to improve it as I'm in charge of the dev infrastructure we use eg:git, code review, computing resources, CI/CD etc.

So I went ahead and implemented mandatory linting and checkstyle as part of the build process, but I did not condition the builds to fail but rather warn.

This is impossible for you to do, it is the team that needs to improve code quality. Unless you're expected to improve it on your own. (And then that's just a lot of repetitive work fixing the problems the linter shows you.)

You've added the linting stuff because you've identified that solving that problem will directly address Bob's desire to improve overall code quality.

So all you need to do is to make the result of the linting analysis for each and every build visible - setup a web page that ties the build number to the lint result.

Share that link. Add in a graph over time of the lint result and the build date.

Let Bob and Alice see this metric that is tied to Bob's aim and the problem will become visible. Then Alice will want to solve it and you don't have to worry so much. Of course, doing this also let's you keep the linter at "warning" level.

1

According to you, Alice's team is breaking builds with "errors that could be fixed with linting and checkstyle". My understanding of these type of linting checks is that they are generally very fast and individual developers can run them themselves on their work machines within seconds. If, for some reason, that does not hold true in your case (linting is slow or it is buggy or is too noisy), then you have much less of a case.

If Alice and her team are refusing to do something which would literally cost them only seconds to avoid pushing bad code, this reeks of a culture problem and sheer pig-headed-ness on Alice's part.

It's possible Alice has her reasons. That is why she should explain them to Bob with you there, so that you can ask for clarifications (as Bob has never been a developer, it's better you are there too).

But make it clear to Bob that there are good reasons for what you are doing. Make the reasons very clear to him. He should know that it is Alice that needs to provide reasons why she is not doing what is considered best practices by the software development industry.

  • 5
    The problem is not the linting, the problem is that he broke everyone's builds. Say I make a little change and it is not accepted because of the 3,000 warnings produced by code that has nothing to do with me. – gnasher729 Aug 7 '18 at 6:44
  • @gnasher729 according to him, they break their own builds anyway because they can't be bothered to do a trivial check. And I don't know how you do your linting in whatever language, but what you describe generally doesn't happen with good linters set up properly. – Chan-Ho Suh Aug 7 '18 at 13:53
  • @Chan-HoSuh My understanding of the situation is that this is old (maybe not "legacy", but at least old) code that nobody has bothered to lint in literal years. Forcing a developer to perform multiple years of linting to commit a trivial change is unreasonable. – Ertai87 Aug 7 '18 at 19:26
  • @Ertai87 agreed, but OP stated explicitly that s/he is concerned about build errors that would have been stopped by a linter, so presumably the lint and style checks would only be on the new code not the old. – Chan-Ho Suh Aug 8 '18 at 3:25
1

The linter should have been in place, as a warning system.

If you were then told that nobody's improving the code according to the linter's benchmark, turning it into a failure based system was the wrong answer.

Instead, you should have submitted small bite-sized tickets to improve the linter output of various modules. Items that could be delivered within a few minutes to an hour of time. This has you tracking their work with the same systems you use to track other bugs / features.

That's apart from the question of "does a linter actually improve code quality?" Some linters do, some only enforce a consistent style; but, since you've made this the "metric of quality" it seems that you'll have to live with your choice.

I've seen systems that attempt to "only make them fix code near where they are working". At first, this seems like a good compromise; but, it really doesn't work in the long run. Most of your code base won't change, and that will be the part with all of the unaddressed linting warnings.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.