Can I criticise the more senior developers around me for not writing clean code?

Question

I am a junior developer. Although I already programmed in different languages for the last 5 years I still have a hard time debugging more complex issues. On the other hand I like clean code and try to write expressive, short functions wherever possible (at present in React). However, it annoys me that around me other developers who are more senior than I am don't write clean code, produce long and snarled methods that are not completely bad but could be easier to read.

My question: Would you bring this up in a meeting - like a 1:1 with another developer? Or would you just keep your mouth shut because you are more junior and wait for the critique until you are senior enough?

Answer 1

Don't wait until you have a senior title to comment on other peoples code. My experience is that the title doesn't matter that much, as does the way you give feedback.

Ask, the senior whether you can share some feedback about the code, or if you prefer can put it as asking questions about the code.

Try to be as concrete as possible and if possible explain the impact it has on you. "I found this method hard to read, because it is very long and does two different things." or "I was confused by this method name, because it says X but the method does Y" is much easier to accept for any developer than "You write bad code"

You might also ask what was the reason for specific coding choices (often enough I found out that "bad code" was actually around for a reason.)

This kind of feedback should be inoffensive enough, and if the developer takes it well there might be a chance for follow up discussions. "Robert Martin suggests this for naming, what do you think?"

But there is also the chance that the developer doesn't agree with you or your favorite author, and doesn't care too much about your opinion. Just safe your energy on that developer, there isn't too much chance you will have a positive impact by being more verbose.

Answer 2

Obviously, criticizing your elders isn't a great move and should be avoided. Belittling juniors is also a bad thing.

One thing to remember is that loose coding practices doesn't necessarily lead to poor compiled code. So, if it works, it works, and the guys know how to maintain their own code. People can, and do, get defensive about their own parts of the code-base.

What you can (and should) do is maintain the best coding practice you can within your own portions of the code. Allow that to speak for itself when other team members see this in the code-base or you're talking through examples.

If you need to work on a part of the code-base that someone else has worked on and you have problems in following it, then by all means ask the developer to talk through the relevant portion so that you can quickly complete this coding task.

You don't have to point out the bad coding practices from other people - take care of your own and hope that other people notice and also take on board what you're doing.

Answer 3

All code should be peer reviewed (but I've worked in a lot of places where that never happened). How clean is clean? There should be coding standards and guidelines; ask for them. As to how "picky" you should be; that depends on the code being reviewed. Some people like having blank lines pointed out to them, and spacing. Others prefer you spot potential problems.

Don't keep your mouth shut because you're junior, but do be careful about how you highlight things. The aim here is not a blame game, but to collectively, as a team, improve the codebase. Try to suggest improvements. The code may not be "wrong", but there's usually something that can be improved, personal coding styles aside.

Answer 4

A few points in addition to the other answers:

Accept that, as a junior, you don't know everything :-)  There may be reasons for the style of code that you are unaware of, such as:

• Avoiding unnecessary changes to working code (keeping diffs manageable, avoiding introducing unnecessary bugs, &c).

• Keeping related code so it can be seen together.  (No benefit from nice short functions if that just obscures the complexity.)

• Avoiding subtle language or performance issues.

• Matching the style of other code.  (Don't underestimate the value of consistency in a codebase; it can make code much easier to read, understand, and maintain, reduce friction between developers, and avoid subtle bugs.)

That said, it's also entirely possible that your changes are worth making!

• Most developers are mediocre (even though we all think we're pretty good…).

• Previous developers may have been in a hurry, and didn't have time to consider refactoring.

• Previous developers may not have cared enough about clean code, or seen how to improve it.

In my experience, every codebase can be improved.  Even on code that I've written and worked on many times, I often spot improvements that I'd previously missed!

As to how to handle it, I'd suggest:

• Don't criticise your fellow developers, or act as if you know more than they do.  Even if you do, that won't win you friends — and a good working relationship is important too.  By all means, suggest improvements, but in a constructive way, without blame.  And accept that they may not agree.

• Run significant changes past a senior colleague first.  If any of the reasons above apply, it's better to find out about it before doing lots of damage!

• Don't rewrite code just for the sake of it.  The original author may take it personally; and you want to be seen as working with them, not against them.  (Remember that what seems an obvious improvement to you may look very different to them.)  Also, each change brings risk; a decent test suite &c can reduce that, but changes aren't free, and being conservative can save headaches.

• Instead, make improvements as you go along: when you fix a bug or add a feature, improve the code you're already working on. That way, it's easier to account for the time spent, and it won't require any extra testing &c. And over time, the quality will increase.

• Finally, accept that while it's important, improving code quality is just one of several priorities.  Developer time is limited, and there are always compromises.  Sad, but true.

It's great that you care about code quality!  Too many developers don't.  But try to use your concern in a productive way that's good for the team as well as the codebase.

—

(I've been that young developer who wants to rewrite everything…  To some extent, I still do!  But I've also watched my own code being changed pointlessly or made worse by people whose priorities I didn't agree with, or who simply misunderstood it.  And I've struggled over codebases which used inconsistent formatting, naming, organisation, and style.  Ultimately, life's much better if the codebase works well together, and the developers work well together.)

Answer 5

Don't get hung up on junior/senior. No one is a perfect developer, and everyone - regardless of title - has the opportunity to improve.

That said, it's important to consider the context. If you're picking out old work that's not really important or relevant at the moment, and then telling them why it's bad quality, that's not going to come off well. On the other hand, if you're in a code review with this person, and looking at code that's currently being worked on, you have a better opportunity. At the end of the day though, you need to understand that employers aren't inherently looking for perfection, they're looking for functionality with a certain level of sustainability. Sure, a function may be rewritable in a way that helps you understand it without a few extra seconds of thought. But that may not be worthwhile for a particular project.

Also, consider that criticize has a negative connotation. No one likes criticism. Instead of,

This is wrong

or

This could be better

you might want to try,

Can you explain this function to me?

or,

Can you tell me why you did it this way?

Those open-ended questions let the other developer talk through their code and their thought process, which may lead to you realizing they had a legitimate reason to do what they did. Or, it may lead them to realizing it could be cleaner/better. Or, it may create the context for you to suggest an improvement. At the end of the day, you've made your point, and with a better chance of maintaining the relationship than if you'd just gone in cold with your criticism.

Answer 6

30 year software development professional here. Perhaps some insight I've gleaned might be of help.

1. Don't sh*t where you sleep. Everyone thinks everyone else's code is crap. This is a pretty natural reaction to reading any code that is difficult to understand. Railing about it, unless you have a good reason to (eg: telling your boss why a feature change is going to take way longer than she budgeted) is just going to tick off someone you have to be able to work with, and make everyone else you have to work with wonder if you're going to say equally nasty things about their code when you look at it.
2. The proper attitude for you to take into discussions about problems with other's code is that you could be wrong. You're probably missing something, right? I try to keep to this, and am still continually annoyed at how often it turns out to be the case. Humans are imperfect and easily confused, and you're a human too.
3. Enlist senior people. Show others (eg: cubemates) the code you're having an issue with. Typically this will result in either an explanation of what its doing and why from someone who's been through the wars, or an agreement and admission that its subpar. In extreme cases even outrage, from someone more fully invested in the codebase than yourself, and whom management will listen to. Either result is a big win for you.
4. If you must complain, complain about behaviors, not people. It isn't that "Fred's code" is inherently crappy. What's crappy is this bit here where it nests 3 lambda declarations inside each other over about 7 screens of code. Doing stuff like that needs to be banned. Can we get the style guide changed to ban this? And names really have to be better than this. We can't have 15 interacting classes all named with some permutation of the same five words (two of which are "model" and "view"). Also, any source file more than about 1000 lines long should probably be split into smaller classes. This routine here that's 1500 lines long should just never happen.^* This is costing us a lot of time making modifications and tracking bugs. Who do I talk to about getting this stuff in the style guide?

^{* - Sadly, all real world examples from inherited code I'm working on right now}

Answer 7

You can criticise the code, no need to criticise the developers. My guess is they want to do better and a friendly comment from a workmate, worded with care, will be welcomed. It's normal to get in a hurry and be a little sloppy--reminders help correct that.

If you've been a developer for 5+ years then the junior/senior demarcation doesn't make a lot of sense and unless your workplace is really aggressively stratified then you are likely better off to not consider it a barrier.

One option is to ask someone to explain what one of their obtusely written functions is doing. Once done you might say something like "Oh, ok, yes, I see that now. Oh, hey! Do you mind if I rewrite this so it's a little more maintainable? That'll help me understand it better and for others later."

Presented that way, you're getting advice and asking a favour -- not performing a code critique -- but with the same outcome. A lot of people will respond with "oh, cool, thanks that would be great. I'll try to be tidier next time."

This can create context for future similar interactions.

Adjust to fit the situation and the person you're working with.

If you find your workmates aren't open then it's unfortunate but sometimes that's just how it is. The more professional they are (senior or not) the more open they will be--by definition.

It is worth making the effort.

I think my main advice is presume these people do want to improve their code quality (this is a code quality issue). But your concern about stepping on toes is always valid and should be accounted for.

Also, it is a part of your job to support the entire team's improvement.

Answer 8

Would you dress this in a meeting - like a 1:1 with another developer? Or would you just keep your mouth shut because you are more junior and wait for the critique until you are senior enough?

I would "keep mouth shut" but not because you are not "senior enough" but because you are not their manager and it is not your job to comment on their coding style.

Here I mean "keep mouth shut" only in the context you have mentioned. You can try casually talk about your cleaner coding style in informal discussions and its benefits without implying that others are not doing what you want them to do.

Another approach, depending on what your team culture is, you could request your manager to allow you to do a "tech talk" or "lunch & learn" kind of session where you go over some new coding ideas you have. Usually teams have these sessions where everyone gets to talk irrespective of their seniority. May be your senior developers could learn something or who knows teach you something on why they do what they do!

However way you approach the idea is to just mention what you do instead of "teaching" others in 1-1 sessions on what they should be doing.

Answer 9

To effectively raise concerns about code quality, a definition of high quality code must be established. Secondly, a code review is an ideal environment to raise these concerns in a constructive manner.

Style Guides

Does your department have a style guide for code? If not, then it's not surprising that your peers' code isn't clear or consistent. You might want to raise the lack of code standards with your supervisor. Emphasize how consistent style improves maintainability by reducing context-switching overhead. If your supervisor sees the value in this, it might be beneficial for you to work on developing these standards since it seems important to you.

Code Reviews

Ideally, stakeholders, peers, and supervisors would have the opportunity to provide feedback in a structured code review process. This would be preferable to booking one-on-one meetings because it provides a more open forum for feedback and isn't as likely to get a defensive response. You might also want to raise this with your supervisor, emphasizing that it's an opportunity for you to learn about why your peers' wrote methods in a certain way, for example.

No matter when or where you approach your coworker, you should come from a more inquisitive perspective. Rather than explain to them why there code is wrong, ask why they wrote a method in a certain way. Even if you are correct, an outright accusation is likely to be met with a defensive response. The old adage, "You catch more flies with honey than vinegar," applies. And if you are incorrect, you will be happy that you were humble in your approach.

Answer 10

Like others have said in several good answers, there are constructive ways of bringing this up with others - asking why they wrote the code like they did, rather than being critical.

I'd like to offer another potential solution: If your team does not do unit testing for the code, you should become a champion for it. Start to add unit testing to your own code, and explain how valuable it is. Since managers hate bugs, they should hopefully get on board with improving testing and testability of the code base. Unit testing (when done right) will naturally force everyone to start writing smaller, cleaner functions. You'll end up with code that's both clean and tested.

Answer 11

1. If you want to become an expertly skilled professional a field, you must first learn to question professionals in that field.
2. Questioning someone is not the same as insulting them, unless you're doing it wrong.

It's both about when you ask and how you ask.

A good time for "when" is one-on-one time, for example while pair programming or during a code review. If neither exist, as a junior you can easily create opportunities for both. Simply ask a senior to review your changes and you got yourself a code review. Or ask a senior to pair program for half an hour, so you can learn more about that unfamiliar component you need to work on.

As for "how": if you don't understand why they would do something the way they did, simply tell them that you don't understand and ask them why the code is written that way. People have reasons for doing things a certain way. Your goal is to find these reasons. Sometimes there are perfectly valid reasons and their code is better than what you would have written. Sometimes the reasons are external (e.g. incentives to optimize "lines of code" "# of unit tests", etc). Sometimes the reasons are just about a minor difference in opinion (are 15 lines too long for a function?). And sometimes the reason is that the programmer picked up some actual bad programming habits.

If you figure out how to ask correctly, many people will freely admit that the code is rubbish and provide you with a list of reasons for why it's rubbish and what's wrong with it. I haven't seen code that wasn't rubbish on some level so far, and I have seen and written a lot of code.

Answer 12

Consider that writing what you call “clean code” may not be at the top of your senior colleagues priority list. And that “clean code” is not at the top either.

Often what is called “clean code” is actually misunderstood - slavish adherence to misunderstood concepts is not improving anything. Often it just adds work for no real benefit, or for very little benefit. So one risk is that you are told why your opinion is wrong- obviously that is a good thing for you.

I had one case where I had written a 300 line method and someone complained it was too long, so I told him to fix it himself. His code was all little and easy to understand functions - with half of the functionality broken. It just hadn’t occurred to him that the function was long and complicated for a reason. Even though it was very well commented (like here we do X to work around bug Y that happens when you do Z - with corresponding code thrown away). Asked why he threw away code he said “I didn’t understand it” (so why didn’t you ask me) or “I thought it wasn’t needed” (so why didn’t you read the comment and try it).

So be prepared that the senior developer actually knows what they are doing and what you call “not completely bad” might actually be very good.

Answer 13

Broadly speaking, "Clean Code" is nice, but not the standard. The standard is the company's style guide. No matter what you learn in some book or at some place, you will step into an organization and it will be different and they will have their own "rules of thumb".

If there's no style guide, then often the code base settles into essentially what the Code Reviewers prefer or what the tech leads are maintaining.

I won't pretend to know every single variable in your situation and I also won't advocate that these things are appropriate. But, nonetheless, they do affect code quality:

Sometimes there's a lot of pressure to push code sooner. To get it to market as soon as possible because it provides a competitive advantage, and in fact, this might be preferable. Martin Fowler talks a little about this in his refactoring book.

You're not always going to have the ideal. The ideal might be too late. However, what you can do is every time you're in a place where you can make a small change for the better, do it.

Broadly speaking if I had one piece of advice when it comes to this sort of thing: Always, broadly, advocate for the cleanest way to do something. However, never leave your ego in the code. Your job is to solve problems and this notion that you think their code is "no clean" is you putting your ego in the code. Learn to be neutral and learn that things are not always as they seem.

Do I think you should approach them? No. Do I think, broadly speaking, you should advocate for "Clean Code"? Of course I do. But change rarely comes from the bottom, work your way into a lead / senior position and then advocate for a "Clean Code" policy.

Simply telling people their code isn't clean, doesn't do much. Also it completely ignores the context of the code. Software development sometimes is more sociological than people think. Context is everything.

Answer 14

When you're a junior, everything looks messy. Joel Spolsky detailed this in an article.

“Is it always this messy?” I asked.

“What? What are you talking about?” the manager said. “We just finished cleaning. This is the cleanest it’s been in weeks.”

OK, so far I’ve mentioned three levels of achievement as a programmer:

1. You don’t know clean from unclean.

2. You have a superficial idea of cleanliness, mostly at the level of conformance to coding conventions.

3. You start to smell subtle hints of uncleanliness beneath the surface and they bug you enough to reach out and fix the code.

There’s an even higher level, though, which is what I really want to talk about:

4. You deliberately architect your code in such a way that your nose for uncleanliness makes your code more likely to be correct.

Seniors often are at level 4. The code is dirty to you, but not to them. It doesn't even matter if it's dirty, as long as it doesn't lead to bugs. People who are familiar enough with their code know which parts are important to keep clean and which parts can be messy.

Answer 15

I don't know this is as much an answer as it is my thoughts on the subject. I think this kind of depends on the code and how 'bad' it is. I've worked on a handful of applications, that have been around for 5+, even 15+ years. The code may not be the cleanest, but it works, and has been working reliably for years.

Different programmers have different views on how to code. Styles have changed over the years. Not everyone keeps up with the newest trends, and some people are just set in their ways. You have to be able to separate style from bad coding practices. In the end, what is important is that the code works reliably.

If this is old code, you might want to look into new code being written and see if that follows better practices. On older applications, you will sometimes see the coding styles change over time. The older code may not be great, but the newer code follows more modern practices.

Look into who is committing what. You may have some coders writing clean code, and some coders writing less clean code.

Talk it out. Don't know that I would bring it up for discussion at a meeting quite yet. Talk to your team mates 1:1 and sort of feel out how they feel about clean coding practices. See if there is any team wide standard on code writing. Bring it up to the lead developer. If they think this is something that should be brought up at a meeting to discuss with the team, great. Some teams care more about coding practices than others. Many teams only care if the code works and passes validation.

I agree with the others. Don't be judgmental. Ask about the code. Ask about standards. Try to learn as much as you can. Make suggestions, but don't push too hard.

Overall, I don't think it is a bad thing to bring up and discuss. You are trying to improve coding practices.

Answer 16

You should not criticize existing code and you definitely should not criticize the coder. You never know how people will react to this and you risk fracturing the relationship and being labeled as a "know it all". Vague criticism, or criticism against existing code is also not productive. There is little chance they will refactor code that has already been tested and potentially deployed.

What you should do is drive towards better practices going forward. Clean code is a positive thing and in a healthy development environment there should be a platform to suggest improvements to code-quality. Generally, this platform is some form of code review. Developers can learn a lot from each other and if there are no practices in place at your company for this peer collaboration then fostering this collaboration is where you should put your emphasis. You can even take the approach of saying that you would like to set up code reviews to learn from them -- and you will.

Answer 17

1. Is code review taken seriously at your company? In training some new developers just yesterday, I made a point of showing a Bitbucket screen where a relatively junior developer (4 years) corrected a very senior developer (40 years). There was even some back and forth. The junior developer was completely correct. If code review is just a formality, bad code will be checked in, and it will also break. Do you think your team's product is stable, or is the code not only poorly written, but results in bad behavior in the field? (Wrong answers, crash, etc.)
2. Does the company have a Code Quality committee? I'm not that excited by long written standards, because I understand the need for exceptions sometimes (Yes, I used goto in the last decade.) But it needs a team that shares your interest in the subject. Join it.

If the answer is that the company blows off code review and doesn't sponsor any group interested in quality issues, you should talk to your manager about this. Those are big red flags.

Answer 18

Seniority generally shouldn't matter that much when giving feedback. You should respect them, their way of working and their opinion either way, and try to balance that with trying to have a high-quality and consistent codebase. With someone more junior you might insist on doing something because you know better (or that's how the team does things), but you should also try to explain why you're suggesting that, to both convince and teach them, which makes it rather similar to how you should interact with everyone else.

Typically code is commented on during code reviews / on pull requests. A 1-on-1 meeting might more make sense if you're looking to discuss the code in more detail, especially to understand it, or if you're mentoring them.

But this could be vary across different workplaces.

When thinking to which degree to criticise the code of others (or doing anything else, for that matter), I would suggest thinking of a new job roughly in terms of the 3 phases listed below. There isn't necessarily a clear line between them, and you may get to changing things quite quickly if you're hired into a more senior position, but it is a good basic way to think about things (and the fitting in phase is sometimes neglected for those hired into senior or management roles).

1. Fit in

Figure out how the team works. Then do things the way they do them.

• What coding standards do they follow?

• How strictly do they adhere to these standards?

• How do they do code reviews?

• How are reviewers chosen?

• Can anyone review anyone's code?

• To which degree do they comment on coding standards during reviews?

• What's the policy/stance on refactoring code?

• How open are they to changing how they work?

  If they're open to it, and you can make a convincing case, you could potentially jump straight into changing things.

These are all questions you can ask, in some form or another, at any point from your first day, or even during the interview process if you really want. You can also find out some of the answers by just checking what people do.

It should always be okay to at least point out a bug or ask how code works or why they made certain decisions, but some workplaces may be exceptions, and anything beyond that depends on the workplace.

If you're having trouble judging this when asking about it or based on what others do, you could leave a review comment here or there and see how that's received. Start with the most significant issues. If it's received poorly, tone it down. If it's received well, you could comment on a few more things.

There's also the chance of finding the way they do things to be much better than how you are used to doing things, or they may have some constraints that make something that would otherwise be good impractical.

2. Prove yourself

You're not going to convince anyone to follow your subjective ways unless you've proven you know what you're talking about.

So prove yourself by doing your job well, writing good code, architecting good solutions, giving meaningful review feedback, mentoring others, getting promoted, etc.

3. Try to change things (if you want)

If you're not happy with how the team does things, this would be the point where you can try to change them.

You can suggest adding or changing code reviews, incorporating some coding standards, adding unit tests, adding automated syntax checking, leaving comments about more subjective things or whatever else.

How do I give good feedback?

Ask questions. There may be reasons why things are the way they are, so don't automatically assume you know better. This is especially true if you're more junior than them, but it also applies if you're more senior. Questioning something may also help lead the other person to the issue without you having to explicitly say it's a problem. "Would this work if X=3?" might be better than "This breaks when X=3", although in that case the difference between the two isn't too big.

Phrase things in terms of "I" whenever you're pointing out something subjective to emphasise that it's your opinion (which they can disagree with without you being wrong). So instead of "This is hard to follow", say "I find this hard to follow". In many cases you can also add "I think" or "I don't think" to "soften" a statement a bit.

Avoid "bad" words like "bad", "awful", "terrible", "hideous", etc. that don't give any information about what the actual problem is.

Point out the problem and suggest a solution. If you just point out a problem, they may not know the solution. If you just offer a solution, they may not know the problem. For example, "I find this hard to follow; you should split it into multiple methods".

"Soften" what you say. I find that criticism comes across better if you "soften" what you're saying a bit with words like "a bit", "maybe" or "might". To modify the above example, "I find this a bit hard to follow; it might be better to split it into multiple methods". This is a bit of a trade-off, as it could also make you come across as a bit more uncertain.

Avoid minor or excessive comments. I wouldn't put a hard threshold here, as necessary comments are necessary, no matter how many there are. But a review with too many comments may be overwhelming or disheartening for the reviewee. If you're leaving many comments, some of those might be related to minor or subjective things that are better left unsaid. A minor spelling or formatting mistake is probably okay to point out if there aren't too many other comments, but if you have a few dozen other comments, it might make sense to skip that one and possibly fix it yourself in that pull request or as part of a later one. Similarly, a comment presenting a different way to do something that's okay as is could potentially also be omitted if there are too many other comments already. Although you should expect to leave a few more comments than usual for new team members.

People are different. Different people may be more or less receptive to feedback. This isn't to say you should treat them differently, but rather understand that what works for a few people may not work for everyone. Try to find an agreeable consistent way to treat everyone, and accept that maybe you have to occasionally tell someone something that won't be received too well.