5

I've became in charge of a group of 3 developers with awful code quality in their project. To increase their code quality, many meetings have been placed and a code quality control (sonarqube) is added to CI to monitor the code and fail the pipeline if it does not pass the requirements.

One of the developers found a way to workaround function complexity limits and commits bad code (example below). My question is how should I approach this to prevent he and other developers from using workarounds instead of thinking and fixing their codes?

switch (true) { 
               case (first & second & otherthing):
                    dosomething();
                    break;
               case (unrelated_if || complex):
                    do_totally_unrelated_thing_to_previous_one();
                    break;
               ...
               }
  • 8
    If you have enough time on your hand, you can force them only to submit pull requests, then you could review those and simply deny bs like that. – Florian Schaetz Jul 6 '17 at 8:28
  • I agree with the comment above. Them having to make requests, also makes that, if you find bs in the code or code that does not meet the standards, you can easily review the code to make sure it does not happen. – Idris Dopico Peña Jul 6 '17 at 8:30
  • 8
    Have you ever asked these guys why they write poor quality code? And if so, what was their answer? – Erik Jul 6 '17 at 8:36
  • 1
    Isn't there any rules on Sonarqube you can enable to forbid switch(true) ? Or you could write your own rule to add it. If they have done something they don't get how to write properly, you should explain that to them, if they're interestind with code quality at all ... – Walfrat Jul 6 '17 at 11:53
  • 3
    The simple answer to your question is "fire them". It's ultimately the only way to get anyone to do anything. Just state "I've mentioned many times that code quality is low. This is a final warning and then we'll have to let go one or all three of you." Note that, on almost any group of about three programmers, about one gets fired for incompetence every six months or so. What's the big deal here? Just fire the worst one. – Fattie Jul 6 '17 at 13:02
8

Personally I find most of those automated code tools useless. There are times when it fails code for things that are simply preference and things that are bad in some circumstances but good or even necessary in others. And often they leave the dev unsure about what the actual fix should be. If you know something fails but don't understand why it fails or what you should be doing instead, the tool itself has failed.

What does help is 100% code review. No code is committed to the production branch without being accepted through code review and no dev has the rights to commit to the production branch only the build team or the lead.

This is where you send back the bad code preferably with an explanation as to why it is bad. The key is to make it painful to not fix the code. Yes they will have a few times where the deadline will be missed because the code failed code review. And they will have to explain that as a reason. This leads people to be less likely to make the same mistake repeatedly so that they can meet their deadlines. If there is no pain to writing bad code, there is no reason to fix it, human nature being what it is.

That said, you and your team need to have an agreement concerning what is good code and what is acceptable code. If your standards and theirs are currently in a mismatch, this needs to be resolved over discussions and an agreeable standard approved. If they have input into the standard (and yes that means you need to compromise and accept their standards at least in part, having the discussion is irrelevant, even counterproductive, if you are still going to dictate end results), that are going to have more buy into actually using it.

| improve this answer | |
  • There are times... Of course there are. This is why most of the tools are configurable. You can, for example, switch of rules, change rule priorities or even add new rules. Static code analyzers can be a valuable tool that shows many typical problems, but they are just the beginning of good code and not the end. It can do some basic tasks, but for more, you will, of course, always need code reviews. – Florian Schaetz Jul 6 '17 at 16:14
  • I'm going to come out in favor of some code tools. Resharper is a favorite of mine. You (HLGEM) may be suffering from the "Curse of Knowledge" - Once you start writing well-formed code automatically, you don't see the value of the tool. However, the tool can be invaluable in getting a developer to start writing well-formed code. My opinion. YMMV. +1 for the rest of the content. – Wesley Long Jul 6 '17 at 16:22
  • Perhaps it is because the only ones I have used are for SQL code and they universally are terrible because they don;t consider meaning and people think their code is good because it passed when the results are wrong and when we had to bypass the rule for performance reasons, it practically took an act of Congress. Additionally, I saw way too many people totally confused about what to replace the correlated sub-query with or unable to make a change and get the same result set. – HLGEM Jul 6 '17 at 17:34
  • Further my experience with people using these tools is that they have to think less which makes it harder for them to move to an advanced level and they do things by rote because the tool says they are bad and not because they understand why they are bad or what conditions the rule might need to have an exception. Making people less analytical is bad for the profession. – HLGEM Jul 6 '17 at 17:37
  • 1
    @FlorianSchaetz There's a difference between running a static analysis tool to look for possible bugs (a good idea) and putting a checkin block based on its output (a bad idea). Static analysis can find some bugs, it will also put up a slew of false positives and conditional issues where it just isn't smart enough to know if something is ok. They also perform pointless nits, and make big deals out of minor style differences. Run them and look at the results, but the only way to ensure quality is actual human review, not automated. – Gabe Sechan Jul 7 '17 at 6:16
18

You introduced a tool that apparently is just getting in the way. The godawful code that you posted has been created because the developer created code initially that wasn't accepted by your tool, and figured out how by making the code worse it would be accepted. That's entirely your problem. If you create situations where people get rewarded for doing the wrong thing, they will be doing the wrong thing.

What we don't know, hearing one side of the story only, is whether they have awful code quality, or whether they have code that you don't like - which can be an entirely different thing. Are you an experienced developer? Then tell them how to improve the code, send them to training courses, and do code reviews. Or are you a pointy-haired boss? In that case, let them get on with it.

| improve this answer | |
  • I have five or six years developing experience while they have about two years. – Reza Salkhordeh Jul 6 '17 at 9:11
  • 4
    In that case, training and telling them how to improve their code. – gnasher729 Jul 6 '17 at 11:47
  • Training should be hand in hand with implementing rules like this - you've set up a pathway for failure, make sure you also give them a pathway for success. – HorusKol Jul 7 '17 at 4:04
  • +1 people do what they are incentivised to do. Set up the system and processes such that they have the incentives to achieve your goals. – angarg12 Jul 7 '17 at 9:03
  • While I agree that this is only one side of the story (although which question here isn't?), that side (along with that code) makes it seems like nothing but insubordination, which can be fixed with training or guidance only if the insubordination is a result of being given unclear rules. If they are insubordinate simply because they don't respect OP's authority or because they disagree (which seems a whole lot more likely, since any 2 developers have a 99% chance to disagree on coding standards), training and guidance will do nothing. – Bernhard Barker Jul 7 '17 at 10:18
4

There are two problems here:

  • their code quality is poor

  • they are working around your code quality enforcer

There is a simple solution: code review.

Review every pull request they make. If they commit poor quality code, explain why it is poor quality. Explain why quality standards are important. Explain that certain design decisions may be faster in the short term, but carry significant technical debt. Explain that deliberately writing workarounds to your coding quality enforcer is unacceptable. The key here is teaching them why it's important, not just telling them what to do. Do not accept the pull requests until they have made all of the necessary changes.

If after a few rounds of this they keep writing poor code and using workarounds, it may be a sign of incompetence or insubordination, which you should address appropriately. In all likelihood, they are not used to writing code in a new style, and need some time to adapt. It is your job as a supervisor to help them learn and adjust, but as the saying goes, you can lead a horse to water, but you can't make it drink.

| improve this answer | |
3

If they refuse to follow the rules given to them it's easy. Give them a warning, in that warning, state that if they get 2 warnings, there are going to be consequences. The fact that you are in charge of them, means that if they continue to do so, the consequences will be going towards you.

Play it safe, make written (via email) rules about what they HAVE to do. If they don't follow these rules, report it to your superior.

Also, make sure u talk to him, there might be something wrong. Writing bad code could be because there is a problem in his work/private space. So make sure that is not the thing, making him committing bad code.

| improve this answer | |
  • -1 This is a very heavy-handed approach. If I were the OP's manager, my first question would be does the code work? Then I would expect OP to make the case that future bug fixing time is significantly increased by current code quality, then I would invest in training for the junior devs. There's nothing to play safe because there is nothing to play. This is a minor issue all things considered – rath Jul 6 '17 at 10:14
  • 3
    If someone else is gonna have to take over the project with bad written code, it's gonna take a LOT of time, effort and money to fix the problems. Not being able to write code witch meets requirements means you are not doing your job right. Making rules makes sure the workers have a chance to make things better, and not worse for themselves and the company. @rath – Idris Dopico Peña Jul 6 '17 at 10:48
  • 1
    @FlorianSchaetz In an idea world, yes, but unfortunately we don't live there. Well-written code is sometimes a luxury. Working code keeps the lights on. It's up to us to find the balance. Detaining a feature or bug fix because sonarqube said so is ill-advised. No tool should get in the way of business. YMMV applies to all of that of course. – rath Jul 6 '17 at 11:11
  • 2
    No, sorry, well-written cod is never a luxury. Badly written code means actively working towards catastrophe, because sooner or later you will reach a point where you cannot maintain it anymore and where it's impossible to fix the bugs or add new features without breaking existing stuff. It's not about tools, but about clean code. And unless you have software that you don't need to maintain (fire and forget software), bad code is only borrowing time from an uncertain future. – Florian Schaetz Jul 6 '17 at 11:57
  • 1
    @GabeSechan I agree with you completely. Sometimes you start with crap code, and any effort to put it in shape will end up wasting more time than you can afford. There's code zen and there's absolute WTF-ery, and somewhere in between there's the Goldilock zone of semi-crappy-but-somewhat-workable. Everyone reads the above and places it in their context, I work with legacy code and I face that dilemma every day. – rath Jul 7 '17 at 6:56

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .