123

I'm a software dev in a small team. I am the most junior member with a couple of years professional experience, but was brought to the (newly formed) team because I had the most hands-on experience with the technology required for the project.

Work has been going well for the past year, but one thing that constantly gets on my nerves is the low-quality code our most senior member commits. I'm personally very strict with code quality and style and always check that all my touched files don't create new issues with our Linter, StyleCop etc. However, he ignores these mostly. He views it as not important and seemingly wants to focus on function rather than form.

Now I don't mind if someone checks in a double blank line or a wrongly positioned brace, but empty catch statements, missing null checks or badly named classes are things that shouldn't go on our main branch, in my opinion. We do not have dedicated code reviews at the moment, and I currently don't see me in a position to push for them.

I normally have no issues discussing this with my peers (we all make errors, myself included), but I have issues discussing this with him. He has been with the company a lot longer and seems kind of sensitive to discussions about his code. In other words, I don't want to piss him off, because I like my current job and I'm likely working with him a lot longer. He's not my superior; we are formally equal.

Should I raise this with my team lead? I'm not sure if I'm just overzealous or too strict here, but considering the product will be very important to our company in the future, I want to strive for quality as early as possible.

  • Comments are not for extended discussion; this conversation has been moved to chat. – Jane S Apr 6 '17 at 2:13

16 Answers 16

120

Should I raise this with my team lead?

No. The most junior member who has only a couple of years experience should not be complaining to team leads about the code quality of the most senior member.

Let the team lead deal with it - there is absolutely no way that you would be telling them something they don't already know. The reasons that they tolerate it (or perhaps even encourage it as a trade-off for faster time to market) might become more evident over time.

If the culture (as evidenced by the behavior of the senior dev and team lead) doesn't suit your quality preferences, you may need to look elsewhere.

That said, even a junior member can lead by example. Make sure your work is top-notch and the quality is beyond reproach. Make sure you hit all your deadlines. Make sure you help others where needed. Maybe others will notice and want to do the same.

We do not have dedicated code reviews at the moment, and I currently don't see me in a position to push for them.

Later on, when you have established yourself within the group as the local expert in code quality, then you can push for process changes. Maybe you could offer to lead code reviews at that time or help refine and enforce your group's coding standards.

  • 1
    Comments are not for extended discussion; this conversation has been moved to chat. If you disagree with an answer, the "answer this question" box is a perfect place to write a dissenting perspective (along with the opportunity to downvote this answer). – enderland Apr 4 '17 at 16:30
  • 13
    This is very bad advice. You are a member of the team you have a vested interest in the maintainability of the code and it's part of your job. You should certainly have a conversation about it with your team lead. Team leads are not omnipotent, we don't read every line of code so they may not be aware of any particular issue. If these are acceptable coding standards then your lead will explain why. But you absolutely should not just wait for those reasons to magically become evident over time. You are very like to find out that the only reason is "because that's how it's always been done". – Justin Ohms Oct 20 '17 at 22:32
48

You are developing code for a business, so you need to think like a business person in addition to (in fact, more than) a developer. That level of thinking usually grows as you gain more seniority.

All the coding guidelines, self-documenting code, maintainable code and other shiny stuff mentioned by various experts in books is great to follow, but if you are going to convince people to start following it, you need a more convincing reason than this:

things that shouldn't go on our main branch in my opinion

You need data to back up your claims. You can certainly go to your boss and suggest adopting all the shiny stuff (being a junior notwithstanding), but be prepared to answer the question, "what would be the benefits to the project by doing that?"

Vague statements like "it makes the code more readable/maintainable/testable/etc." (or much worse, "so and so recommends it") doesn't convince anyone to change their tried and tested methods.

Remember that the "poor" code of your seniors has been getting the job done for ages. "Getting job done" is among the most important things in business. Telling people there is a better way of getting job done with only your "opinion" to back it up will not convince them to take the risk.

I would certainly encourage you to push the team to adopt the shiny stuff, but be prepared to put in the effort required to gather the data to support your claims.

As an additional tip, I hope you won't use the tone you have used in this post when you set out to convince your seniors. "Ha ha, you guys suck, I will teach you how it is done" will definitely not work, "I have a suggestion to improve X, and I think these are the reasons why it would benefit us. What do you think?" has a far better chance of getting accepted.

  • 33
    Not sure if it's your intent, but talking about "shiny stuff ... in books" might come across as if you thought that content was somehow meaningless. The very books and other industry experts that have been around for decades are the data to back it up. In that case, it's the OP who has to summarize his concerns (e.g. "working with X's code is more difficult, time-consuming, and has more bugs than other code I've worked with where best practices are followed, etc".) They won't go read whatever he suggests and they won't care about code quality b/c they don't need to work with themselves. – code_dredd Apr 2 '17 at 23:24
  • 6
    @ray I think what s/he's trying to say as that theory is great on paper but when you have a client breathing down your neck wanting results now, theory and glittery code need to take a backseat to productivity. And in my admittedly short time in this industry, productivity and quality are not always correlated. – 8protons Apr 3 '17 at 0:00
  • 15
    @8protons All clients always want results "now", but this is not limited to the software industry. Airlines also want their airplanes "now" and for less money; should their engineers be excused for cutting corners on quality because of that? Aircrafts also rely on software, so it's not like this is limited only to the physical frame... and I'm sure you'd still have an angry client breathing down your neck if the software is not stable. Quality will surely hit that spot. If you define "productivity" as "quantity", then spending 100% of your time fixing bugs would also be very "productive". – code_dredd Apr 3 '17 at 0:15
  • 6
    @ray All I'm saying is that I've seen people write good code in X hours and others write near-flawless, lovely looking code in 2*X hours and at the end of the day, the boss was more happy with x and the client was more happy with being charged for X hours. It's all circumstantial. You can write solid code without it having to be absolutely bullet-proof – 8protons Apr 3 '17 at 0:17
  • 9
    @8protons I get what you're saying, and you're right: there is a cost for better quality. I wouldn't debate that. What I'm saying is: that is true everywhere in everything. "It works" is not the only relevant measure. Even the Titanic didn't sink immediately after touching the water; the engineering failures were exposed after tragedy had struck. My point is that cutting corners now will cost you later on accumulated technical debt. Future tasks will take progressively longer to complete, etc. The bill always comes due, sooner or later. – code_dredd Apr 3 '17 at 0:30
30

I did experience this in my previous company. It did not end well.

We were two writing the code and the senior colleague was a huge fan of copy and paste programming. ("Remember, there is no problem about that.") The same block of the code was scattered about 50 times around the code base, following the same thought template. I could live with that although I did not like that very much. Null checks through try...catch with empty exception handling were common. After some time I started refactoring that stuff and I was told I am obstructing the work because after the changes he is no longer able to program "from his memory", relying on the older structure. Despite the quality of the fixes and removal of many bugs, I was told "I damaged the code" because in business, no one cares about minor code issues and the only criteria is to get the job done in minimum hours and get satisfied customer. Working with the code led to my unsatisfaction and I was also missing deadlines not able to "copy-paste" fast enough, because I felt I do not want to become such a "fast and straightforward programmer" as I was expected to be. Finally I left and found other company which met my expectations on what I considered to be standards on the field.

I understand there are some "practical" people among programmers. They are often very productive, not paying much attention to details. If you do not like this approach, consider whether to stay in the company or leave.

  • 11
    I think possibly your "mistake" was changing code you had no remit or reason to change. I do the same thing but only when I've been told to make changes in that area. At scrum, when I declare what I've done, I say "in performing this change, I observed a lot of unnecessary code duplication so I refactored this to allow us to make changes to this area more quickly in future". It's never been an issue. Of course, company culture could play a part too. – Michael Apr 4 '17 at 10:14
  • 1
    @Michael - I did not mention details but I was changing only classes where I was doing some other changes similar to what you said. You mentioned scrums and we had none. If there were any, we could probably better discover (and agree on) which changes are acceptable and which are beyond the necessity. This could have been caught if there were code reviews in the place, but we had none. It was a big lesson how to not do some things. – miroxlav Apr 4 '17 at 23:59
11

I do not think you can a lot in the situation. Raising quality issues with your team lead against a senior member can create lot of awkward moments and conversations. (Unless there is a major bug due to which the code will not run at all). Otherwise you may be perceived as someone who has high opinions about himself (or low opinion about others).
I see your concern and I think you should be able to raise these issues but in my experience these things generally do not go well.

My suggestion would be do your best to maintain the code at the quality you desire and let others do what they are doing. Also, from time to time you can causally talk about code quality without pointing fingers at anyone specific. At some point people will notice your efforts and they will have all the revision histories to check. They will recognize you for this. Also, you may be at a relatively senior position in sometime where you can "demand" better code quality from the team.

At this point, I think you should take the extra effort and do best to maintain the code quality yourself and show everyone you are sucker for a clean code!

8

Firstly, your coworker might know what he's doing and have reasons for it. For example, if you're doing agile development and showing weekly prototypes to clients, then it might be a perfectly legitimate strategy to implement as much functionality as quickly as possible so that the clients/users can see the design and give their views on it, and then worry about edge cases, error cases, and code quality later when you know that you've got agreement on the requirements.

Secondly, your coworker may be someone who is good at seeing the big picture and bad at following through with the small details. I've been on a number of projects where that kind of person had the vision and set the direction and made a great contribution to the success of the project (though such people often end up not writing any code themselves). There needs to be someone else in that case who is obsessive about the detail. They might (or might not) recognize that description of themselves, especially if you present it to them in a positive way. Try to find out, without causing offence, whether their assessment of their own strengths and weaknesses (and therefore, where they need help from team-mates) coincides with yours.

Thirdly, try to concentrate on the weaknesses that have tangible manifestations, rather than becoming obsessive about "the way you think good code should be written". Avoid subjective criticism (no two programmers agree on all aspects of good style.) And concentrate on the areas of the code that you have a legitimate interest in. Write test cases that fail, and log bugs against them. Add comments or TODOs to the code like "Need to handle I/O failure here".

  • None of this justify empty catch statements and swallowing errors! – Ian Apr 3 '17 at 18:45
  • 1
    If you leave those comments, don't make them passive-aggressive. I recently had the schadenfreude of watching someone else suffer because their teammate kept leaving comments like // It'd sure be great if we knew what this did, as opposed to // TODO Document `fooBar` field and its property. Likewise for // I wonder what this does if the user presses five keys at once? -- that should be // TODO Will crash if user presses five keys at once. Also, remember to log bugs in your bug tracker, not just TODOs in code. – Nic Hartley Apr 3 '17 at 22:49
  • 7
    "... it might be a perfectly legitimate strategy to implement as much functionality as quickly as possible". In my experience, this kind of "we'll improve the quality later" mentality doesn't work. It will get delayed further and further until you've got an unmaintainable mess. If you've pushed out an entire (albeit crappy) prototype in 1 month, your customer will likely develop unrealistic expectations about how quickly you can deliver in future. – Michael Apr 4 '17 at 10:19
  • Pretty sure the above comments go directly to the posters point of no two programmers agree on all aspects of good style. – Matthew Whited Apr 4 '17 at 14:11
7

You say you are strict with code quality, but you are not in charge, so that doesn't matter from the company perspective.

What matters is the company's stance on code quality.

If there is a strict company policy, you can use that to back any discussion with your coworker, but trying to go to your supervisor will create a ton of conflict, so it's best avoided for general style concerns which won't lead to disaster.

If there isn't a strict company policy on this, the only things I'd even consider bringing up (with either your coworker or supervisor) are things that are likely to or have in the past led to the code breaking.

First approach your coworker. Do not tell him the way things need to be (because you should not assume you know better, and there's no faster way to make someone defensive). Ask him his reasoning behind doing it his way and respond with an argument addressing the problems with that, if you can find any (perhaps he has a good reason for doing things the way he does).

If that doesn't work, you can consider bringing this up with your supervisor.

Don't be accusatory or even mention the other team member at all - just bring up any one of the issues and point to one or two examples in your code base, and proceed with the same humble approach mentioned above.

Your supervisor might check or ask you to check who wrote the code (if possible) and this might lead to your supervisor addressing the issue with said coworker. But all of this would be at the discretion of your supervisor. Yes, this might lead to your coworker thinking you "ratted him out", so proceed at your own risk.


Coming from an experienced programmer, all of the things you say should ideally be avoided, but none of them are necessarily at the "WE'RE ALL GOING TO DIE!" severity. Ignored exceptions or missing null checks can be disastrous, but they can also sometimes be functionally unnecessary or intentionally omitted. Naming of classes doesn't affect execution at all in any language I know of (reflection notwithstanding) and is thus much less severe than the above concerns which can cause problems at runtime, and it's also generally entirely opinion-based.

So I feel it hurts your argument to put all 3 of those things at roughly the same severity.

But you're certainly free to refuse to work in an environment that doesn't meet your standards and escalate any style concerns as though it were life-threateningly serious.

  • 9
    Having poor code style in your code base is a 'death by 1000 cuts situation'. As you say, nothing on its own is a 'disaster' but when you cant diagnose an error because an empty catch{} swallows it you will feel the pain. When you it takes you twice as long to maintain your own code when you come back to it next year, you will feel stupid. As professionals we should refuse to work in environments where basic levels of code style are not policy for new code. – Gusdor Apr 3 '17 at 9:56
  • 2
    @Gusdor You can refuse to work wherever you want, but it will be impossible or really difficult most of the time to impose your own coding standards at a company where you're a junior (or even a senior) developer. – Dukeling Apr 3 '17 at 13:40
  • 3
    @Dukeling Imposing your own standards? I agree. Imposing established, verifiable standards with mature tools like StyleCop, FXCop and MISRA? Much easier in my experience. – Gusdor Apr 3 '17 at 13:53
  • 2
    I think this hits the nail on the head. A company exists and produces product to make a profit. If the business manager's policy is that functionality over form is the best for maximal profit then you need numbers and stats to convince them otherwise. It's company policy that needs to change here to achieve your intended result, not a single persons attitude. – Toby Apr 4 '17 at 12:22
  • 1
    If the person is senior and has been producing code like this for some time then the higher ups obviously find the end result at least sufficient to meet their business goals. Your attitude in this should be to forget about the colleague, switching instead to look at how does your aim in this help the company? If you think that it will - prove it to the higher ups (without reference to any colleague) and have them institute any change they think is required. – Toby Apr 4 '17 at 12:22
6

We do not have dedicated code reviews at the moment, and I currently don't see me in a position to push for them.

Yes, this is what you need (even if you don't think you can).

Anything other than regular code reviews won't resolve those issues.

If your team is large enough, find someone else to have weekly code reviews with. You only need two people to get started. And when a new developer gets hired, have them join in. Code reviews are actually a great way to do onboarding. Of course, this won't solve your immediate problem, but assuming you're able to find another teammate who's willing to participate, it should be easy to get your team lead's permission to start getting the ball running.

Just don't count on that other existing developer to join in unless you already have a large number of team members already participating (which could take years, but ultimately, you have to get started somewhere, otherwise you'll never get started).

And be sure to help run those code reviews as efficiently and as non-judgementally as you can. Even if the team lead offers to help, don't count on him to research the topic as thoroughly as you do and lead those code reviews as efficiently as you can, since this is basically your idea to give it a try.

Again, this does not solve your immediate problem. You wanted to improve his code, not just yours. Unfortunately, new habits like this don't just appear overnight. They need time to develop.

Regarding Masked Man's post:

As to needing data to back up your claims regarding null checks, swallowing errors, bad class names, etc. The book Code Complete has all the data you need to back up such claims. Plus, you could probably track some of those things within your code base by using your current existing analytics package. For instance, you could count the number of times your application fails silently, even when there is a critical piece of information missing.

But I'd suggest you don't waste any of your time doing that. It's simply faster to fix the mistake than trying to keep track of the consequences of that mistake. And since that developer's ego is on the line, data won't change his mind anyway. The same goes with your team lead. Even if he agrees with you in principle, he has to weigh the benefits of good coding practices against the disadvantages of having ongoing arguments with your particular teammate.

  • I agree with starting code reviews with another team member, especially one you respect. – Gustav Bertram Apr 4 '17 at 14:04
4

Institute mandatory code reviews.

You're not in the political position to just demand such a policy shift, but you could propose it to other members of the team and get their buy-in. Once a significant majority of you agree, bring it up with the larger group. Here's a great reason why you should be doing them: https://www.atlassian.com/agile/code-reviews

Our office does this and I love it.

4

Although code quality should be important, it needs to be put into the context of management's expectations and the skill level of the team members. Wanting to improve in this area is a good thing, but focus on the problems and not just the symptoms.

Everything you are suggesting is done for a reason. Are users getting errors due to a lack of null checks that require debugging? Every programmer should know the earlier these are caught the easier they are to fix.

After you make the connections between these coding flaws and the problems they are creating for everyone, your team should start looking for ways to fix them. Code reviews or some process to get more than one set of eyes on each line of code is considered a good practice. Where you will run into trouble is if there is no enforcement of what you team thinks is best. If one programmer is allowed to do things his way, none of this is going to work.

At some point, it should be apparent that this person't code is causing more problems than the code created by others. It is up to the team to address problems with sub-standard performers. Hopefully, everyone can come to some consensus on these issues in pursuit of better code.

3

This is a teachable moment

I think it is OK to approach the team lead if you position this as a learning opportunity for you.

Tell the lead you are concerned about code consistency, and want to make sure that you are following the proper standard. Should you be writing what you think is a proper level of code quality? Or should you be doing something more like that senior dev? Perhaps you are overbuilding.

By bringing it up this way, you avoid confrontation yet ensure that the team lead is aware of the issue.

P.S. I suggest you don't press for an immediate answer, and be ready to let it drop.

2

It doesn't sound like the senior colleague is the problem here. Humans will often take the path of least resistance if given the chance. Your problem is that the process doesn't enforce the quality standards that you deem to be necessary.

Grant me the serenity to accept the things I cannot change, the courage to change the things I can, and the wisdom to know the difference.

The issue you are struggling with is that you don't know whether, as a junior, you're in a position to have any influence over this process. Can you make them change or can't you?

I'd basically try to glean information in a casual conversation (or several) to figure out what they've done in the past and what their objections are to the practices you're suggesting. You'll likely be able to get a sense from this alone of the things which you're likely to be able to affect.


Something which you probably do have direct influence over would be asking more senior team members to do informal code-reviews of your code. Remember, you're a junior and so therefore you are useless - having a more senior member of the team check over your work will surely reduce the amount of bugs you introduce and help you to learn*. I think it's unlikely that you couldn't get one person to agree to this.

By doing this, you'll not only have greater confidence in your own code (even if you don't have it in the code of your colleagues) but perhaps you will start to foster an atmosphere where code reviews are seen as a valuable part of the process.

*I'm being facetious.

  • +1 for "your problem is that the process doesn't enforce the quality standards that you deem to be necessary" alone. – SQB Apr 4 '17 at 20:48
1

In my experience, code styling is really hard to enforce and always leads to many unfruitful discussions. It's a subjective matter and some people have really strong opinions about it. If you really care about it, you should convince your team to implement an automated build system (e.g.: Jenkins) and add automated source code validation on it. If the code is not linted, the build goes red and won't be accepted.

Let your senior college arge with the machine instead of his junior college.

  • I guess this comes round to the same problem. How do you, as a junior, encourage them to use an automated build system if you can't encourage them to do code reviews? They have their process and it's "working" as far as they're concerned. – Michael Apr 4 '17 at 10:25
  • 1
    It's different. The OP stated that the seniors are not following the Linters, so I guess that means that the project has Linters but the seniors went rogue and are not following the rules anymore. Also, when presenting this idea, he should focus on other benefits of the build system and not only the linting. – Cesar Canassa Apr 4 '17 at 15:16
1

Nobody likes a tattle teller.

You worry about your own code, and very soon your colleagues will see you as taking ownership of the product.

I'm experiencing a similar dilemma where I'm the senior developer and my peers are junior, many of whom aren't interested in quality.

Time will weed out those who aren't passionate about what they do.

0

Should I raise this with my team lead?

Yes.

However, before doing so, read on and make sure you raise the right "this".

Indeed, as the junior member, it's not your place to say "this other developer is writing bad code". However, there is clearly a difference in what you consider quality code you consider acceptable and what code other team members consider acceptable. That is something that would be good to discuss with your team lead. This way, it is much less about faulting someone else and more about finding a good way to continue from here. This may mean that your team lead tells you that you can relax your coding standard. It may mean you end up finding the correct middle ground.

Either way, you feel uncomfortable because of the differing standards people hold their code to, so that should be what you discuss. It shouldn't be whether or not someone else is writing code that is acceptable or not.

We do not have dedicated code reviews at the moment, and I currently don't see me in a position to push for them.

Indeed, you are not in a position to push for code reviews. However, you can suggest using code reviews. It may even be the case that they recognize what advancements can be brought in through a "fresh pair of eyes" and using dedicated code reviews may be exactly such a thing.

0

Another way to bring this up without getting on anyone's bad side would be to approach the issue obliquely, as one of the team's development process. Identify some past bugs that were observed in the code base that respecting compiler warnings or other tools would have caught before they caused trouble. You can then take that to the team lead and even the whole team, and suggest that the relevant kinds of warnings from those checks be made into errors in your build/test process. No one likes having to re-work code after they're written it and moved on. You can present this as an easy way to make sure you're all getting things right the first time, and avoiding likely bug reports.

Over time, you can mine more classes of bugs that the checkers would have prevented, and get them added to the enforcement list.

As an additional benefit, this approach also avoids putting effort into fixing complaints about the code that consistently turn out not to matter for your particular application. The warnings may not be false positives in the sense of being wrong, but they may be false positives in the sense of not making a difference.

0

The word for this is root cause analysis. If you can prove that these commits are the source of one or more major bugs, then discuss these commits during the team meeting or defect review meeting as a group to raise the awareness of the negative impact of these techniques. If that is not the case, you can also just email out a snippet to the team and ask someone to provide an explanation, so at least you can improve the documentation.

References

protected by Lilienthal Apr 4 '17 at 8:20

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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