102

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?

Improve this question
6
  • 85
    Could they be hard to read because the problem and requirements are complex to solve? You might ask them why a given method was implemented that way. – Juha Untinen Apr 24 '19 at 9:41
  • 11
    I am kind of curious what makes you think their code is bad. Is it simply that they write long functions? is it not DRY? Poorly formatted? Everyone seems to have some pain point early in their career that contributes to what they think is good and bad, perhaps yours just isn't the same as theirs. For me as long as the code is DRY, I don't much care how complex it is, but others have had different experiences. One of my other trigger points is "Short, Expressive" code which often seems to mean "Heavily Encrypted". – Bill K Apr 24 '19 at 18:37
  • 5
    You say you've been programming for 5 years but you also say you're a junior - can you clarify on that? – Benjamin Gruenbaum Apr 25 '19 at 9:36
  • Hi @BenjaminGruenbaum... Well I don't know if there is a standard for the labels Junior, Professional, Senior. I only think I could still improve and see that the people around me are faster than me. I subjectively have the impression that being a senior takes more than 5 years and differs also from person to person. – Marc Apr 25 '19 at 10:40
  • 2
    @BillK Well I think I go pretty much with what Robert C Martin and Michael Feathers teach. It is easy to write code that computers understand but hard to write code that humans understand. – Marc Apr 25 '19 at 10:43

18 Answers 18

Active Oldest Votes
3

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.

Improve this answer
5
  • ...often enough I found out that "bad code" was actually around for a reason... Most of the times, this is just to cut corners. For an example same code being duplicated just to skip the time that needs to be spent planning and thinking on how the function should be written so that it is reusable. – Romeo Sierra Apr 28 '19 at 19:42
  • 3
    I would focus on the reason why Robert Martin (or any other person) makes a given suggestion, as opposed to simply appealing to a particular authority. – mcknz Apr 28 '19 at 19:49
  • 2
    "You might also ask what was the reason for specific coding choices " It can be very hard "asking" a question like this without revealing your underlying disagreement . I think the better approach is saying what you think is better, and the response will contain a counter argument without you asking for one, and either you'll learn the reasoning behind the original code, or you will manage to continue with a debate until some conclusion is met. – NiRR Apr 30 '19 at 12:03
  • I see your point. I agree, that it isn't a good practice to camouflage critique as a dishonest question, but in my experience it often does help to assume that there was good reason to do something a certain way and honestly try to figure out what it was. – Helena Apr 30 '19 at 18:02
  • If you use GitHub or something similar, reviewing other PR's voluntarily can be a good initiative. – user47813 May 1 '19 at 17:53
135

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.

Improve this answer
10
  • 14
    In the 1st line. I suggest changing Obviously by Arguably. Some elders consider criticism as a great value. Or maybe changing criticizing by belittling; it is indeed not a great move then. – Anonymous Coward Apr 25 '19 at 15:32
  • 17
    "So, if it works, it works" - most likely it doesn't work, but nobody knows it yet. "and the guys know how to maintain their own code." And eventually those guys leave, and then my company gets contracted to take over b/c nobody else can figure it out. Not that I'm complaining exactly, it means that my company will never run out of work :-) – industry7 Apr 25 '19 at 19:30
  • 5
    loose coding practices doesn't necessarily lead to poor compiled code Why does this matter? In my understanding the complaint was not at all that poor source code leads to bad compiled code. if it works, it works I could write the same piece of code in 100 different ways, which would all "work" in the same way, and then show you the 20 worst versions and challenge you to say that they could be kept because they "work". In a professional context a piece of code "works" if it delivered the expected result and maintaining it is not a nightmare and doesn't require the authors to do it. – SantiBailors Apr 26 '19 at 12:29
  • 3
    To suggest that everyone should just "do there own thing and worry about their own code" is terrible advice. Imagine giving similar advice to a sports team, for example. It's not how teams work. – aw04 Apr 26 '19 at 17:17
  • 8
    I get the impression that most people up-voting this answer have little to no idea about software development and how problematic it can be to have to deal with someone else's crap code that only the original authors understood and no one else. "It works" is a necessary, but not sufficient, precondition for good code. It's ironic that this answer was written by someone who is a "developer working for one of the largest software companies in the world" (as per profile). – code_dredd Apr 26 '19 at 18:35
102

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.

Improve this answer
4
  • 12
    Yes, the first thing to do is to push for a proper code review process. That way everyone is invited to provide feedback and it doesn't feel so much like a personal thing. Once code reviews are in place, ensure the responses are not overly critical but instead are framed as possible enhancements (don't say "this is unreadable/hard to read" when you could instead say "this might be easier to read if you did XYZ"). – delinear Apr 24 '19 at 9:10
  • To avoid having to point out whitespace issues, make sure everyone has a linter plugin on their editor and if any issues get committed just add a comment saying there were lint errors added. – Qwertie Apr 26 '19 at 3:21
  • 2
    Agreed here... I would talk about best practices rather than focusing on comments on specific code that may come across as negative or personal attacks. – JeffC Apr 26 '19 at 13:49
  • 1
    This is a much better answer than Snows, as this one actually suggests that seniors and "seniors" should find and follow good coding practices, rather than just write a bunch of junk and assume everyone else can understand it and that it's good simply because "it works for me". I've seen senior devs write code they couldn't understand a week later, as well as novices write very understandable code, even if it was a bit "simple". – computercarguy Apr 26 '19 at 17:28
96

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.)

Improve this answer
11
  • 25
    "Don't underestimate the value of consistency in a codebase; once you're familiar with it, it can make code much easier to read and understand." Thank you, I've had this discussion multiple times with people that wanted to refactor some small part of a huge legacy project. They usually had valid concerns, but at one point consistency is more important imo. – Ivo van der Veeken Apr 24 '19 at 10:00
  • 5
    I'd suggest adding one more "Don't do rewrite team code without running it by a senior first." Junior dev doesn't want to find themselves a couple of months in (if senior devs don't check commit logs carefully) having rewritten massive amounts of team code one small bit at a time only to have angry senior devs showing them exactly why their changes were very bad for some reason (broke something, made the code unmaintainable, etc.), and now there's a big avoidable mess to clean up. – bob Apr 24 '19 at 14:49
  • 7
    Why does this answer have random "&c" characters in several places? – Aaron Apr 24 '19 at 16:34
  • 9
    @gidds because I was curious and others may be likewise, I looked into it then asked about it on English.SE: english.stackexchange.com/questions/496087/… – Aaron Apr 24 '19 at 21:35
  • 5
    Using &c is a really good example of a rarely used construction confusing others, instead of just using what everybody expects - "etc." or even "and so on". A peer review would probably have flagged that... – Thorbjørn Ravn Andersen Apr 25 '19 at 22:02
29

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.

Improve this answer
5
  • I like "Can you tell me why you did it this way?" in particular. It's the least offensive as it frames the question as "I don't think there is anything wrong with or code, I'm just trying to learn in order to get up to your level." But then you can always follow up with "Oh, the way I would have done it is like... and I don't understand..." And again your'e not saying that it would be better to do it your way, but simply opening up communication and getting the coder to elaborate in a way that will end up highlighting any issues, if they exist. – industry7 Apr 25 '19 at 19:37
  • 1
    This is also an extremely important point: "they're looking for functionality with a certain level of sustainability" You're working for a business that needs to maintain a profit, and there's a difficult line to tread when it comes to maintenance costs. How good is good enough? How much time do we spend now to avoid potential future problems? It would be great if we could get hard objective numbers about how these choices affect the bottom line, but it's all so complicated that there will probably always be a measure of subjectivity to it. – industry7 Apr 25 '19 at 20:00
  • Upvoting because this is the first answer that mentions to frame the critique as questions. Striking the right tone is the most important aspect here. – blues Apr 26 '19 at 9:46
  • Always initially assume there was a good reason. (You can later privately decide for yourself if you want!) A good initial assumption is that it was due to time constraints of one kind or another. Writing clean code in itself is not a categorical good: it is hypothetical on the basis of some organisational need. The biggest additional constraint is usually time, whether that's the time to write code, fix it, understand context, etc. The senior is probably well aware this and of their limitations. I'm afraid this won't be news to them. As a junior developer there's a great opportunity to help. – Dannie Apr 26 '19 at 10:55
  • I don't remember where, but I read that devs spend more time reading code than writing it. If the code takes minutes to figure out and can be rewritten to take seconds to understand, then it might be worth it, especially in code that gets read often. As someone else stated, it's a difficult line in maintenance costs, which comes down to speed of development and making good code that's easily readable. And sometimes there's a major performance enhancement that's gained by making it complex. It's not as often as some devs like to think, though, so YMMV. – computercarguy Apr 26 '19 at 17:36
17

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

Improve this answer
2
  • 4
    I've seen code files 16,000 lines long... but I've also seen junior developers who think they know everything, trying to convert a perfectly readable and stable codebase into "microservices" or "functional programming" or whatever the newest fad is. Hard to know "who is right" here... – vikingsteve Apr 26 '19 at 7:58
  • "What's crappy is this bit here where it nests 3 lambda declarations inside each other over about 7 screens of code." Pffft... grasshopper. :p – AnoE Apr 26 '19 at 13:20
15

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.

Improve this answer
1
  • 2
    Good advice. Before you consider doing this, make sure that the "senior" isn't following an established/existing pattern in that part of the code. It may not be how they would have coded it, if they had coded it all themselves from scratch. Also, be ready for the fact that you might have wished you kept your opinion to yourself (said with a warm smile, not malicious intent). Best wishes! – J. Chris Compton Apr 24 '19 at 18:51
5

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.

Improve this answer
3
  • 2
    The more casual approach is definitely a good way to handle this sort of thing - arranging a meeting sounds far too serious. Preferably the team should provide feedback in a structured manner though, as in line with code review principles. – Erdrik Ironrose Apr 24 '19 at 8:35
  • 3
    Managers can't comment on coding style at all. They don't know how to code. It's not their expertise. So it can't be their job. – Gherman Apr 24 '19 at 9:03
  • 3
    @Gherman This is highly dependent on the work environment. In the last couple of places I've worked, our direct managers were probably our most competent coders. Some places promote people into management instead of finding business-only managers. I'd say this answer would fit well with that type of workplace. – Booga Roo Apr 24 '19 at 9:58
3

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.

Improve this answer
1
  • 1
    +1 because this suitably addresses the ambiguity in "clean code". Asking "is the code clean" should really be "is the code highly covered with linters, unit, integ and e2e tests?" "is the code tracking in Git with a CI/CD pipeline?" "are daily builds being done?" Those should be answered first and then cleanliness is mostly automatic. – ignorance Apr 25 '19 at 23:00
2

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.

Improve this answer
2

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.

Improve this answer
1
  • 100% agree with this answer (and actually wanted to write exactly this), it's not about seniors or juniors but about the development methods in the team. Don't criticize colleagues for following the practices of their team (and if there are no processes since the team lead decided sw quality is not important, then it's not the mistake of the colleagues) – Sascha Apr 25 '19 at 7:31
2
  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.

Improve this answer
1

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.

Improve this answer
1

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.

Improve this answer
1

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:

  1. 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.

Improve this answer
1

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.

Improve this answer
0
  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.

Improve this answer
-4

Almost all answers here are by conformists. They tell you to be diplomatic and not to push the topic too hard with your colleagues. Most people would agree to that and will act accordingly, resulting in the average (worst) agreement on code quality that they can get along with. However, (code) standards are here not without reason but because they stand for themselves in order to improve maintainability, readability, etc. All of which are long term goals that most developers don't care about because they don't feel the negative or positive impacts of it while writing the code.

Higher standards should never be oppressed by some kind of laziness or appeal to authority. If while raising this topic to the "elders", they feel attacked and act defensively you should probably look for a place where you can live up to your standards. After all, it is you who is giving the most valuable thing: your time. Money can be earned anywhere. Time is precious - and bad code is taxing your time.

Improve this answer
8
  • 3
    workplace.stackexchange.com/questions/135337/… Before fighting for a cause make sure this really is a cause worth fighting (and possibly dying) for. I've seen too many hot-blooded juniors fail this check to even be angry about it. – ivan_pozdeev Apr 24 '19 at 12:43
  • 3
    when the battle is uphill, you have to be very, very, very careful. Strategy is the art to attack only in position of superiority, and he's not in that kind of position. – gazzz0x2z Apr 24 '19 at 13:29
  • 1
    Two key things I've learned about code quality: 1) Code does not need to be "perfect", it needs to be "good enough for the task at hand". Making it "better" than needed, you are being inefficient, wasting your time without adding value -- the opposite of what you are trying to be. 2) Code quality is not an end in and of itself, it's a means to an end -- implementing features faster, introducing fewer bugs. So a convention is only worth following if in the specific environment, at the specific time, the additional time spent on it will measurably pay off. – ivan_pozdeev Apr 24 '19 at 13:38
  • 4
    I firmly say pointing out someone's coding style as wrong will not go over well. I maintain COBOL applications, some that are older than me. Our rule of thumb is to continue the style that came before us. Some of these system as massive. Even though our code is not "clean" everyone understands because it is uniform. Its not about conforming, its about understanding and just because a junior finds their code cleaner, does not mean it meet efficiency needs, or even be cleaner. Maybe they just understand it better because they wrote it. – SaggingRufus Apr 25 '19 at 13:00
  • 2
    In a way, this answer is very Meta. It's an undiplomatic and abrasive answer by a new contributor that insults the people that have been on the site for while - and one that was negatively received by the community. And since the answer is basically suggesting being undiplomatic/insulting/abrasive towards senior coworkers at a place where you earn your living, well... you have a lot more to worry about than simply having a downvoted answer. – Kevin Apr 25 '19 at 17:29

You must log in to answer this question.

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