37

(I'm talking about HTML / CSS code (not programming languages) but I think we also face the same issue as with programmers.)

I am the senior front-end designer in a team and I often have to re-work my juniors' output in tight deadlines.

I am faced with 2 problems:

  1. Their coding style is a bit of a mess.
  2. The aesthetics are not good.

Their coding style, I find, is a mixed bag with no proper convention / standard. I am torn between cleaning up the code or just dealing with their code (even copying how they do things).

I do find it frustrating to follow their coding style as I feel I might learn bad habits. But then, that is the fastest way of meeting the deadline.

For those with much more experience, which is more effective? Should I save the clean-up for later? Or clean-up along the way as I make the changes?

(I don't want to sound arrogant though but such is the reality. It will take them more years to write better code. I know, I wrote messy code when I was starting.)

12
  • 1
    If you have the option, look into JetBrains products (Re-Sharper for C#, IntelliJ for Java, and even some 'dynamic' languages) which can do project-wide idiomatic changes solution-wide, with very little investment of time. (It also can be used to interactively teach the junior what is idomatic. But make sure you and they agree on the same settings for the project. (And make sure you do all that stuff in a separate commit, so you don't mix substantive and cosmetic changes up in the same commit), Commented Mar 17, 2014 at 5:01
  • 1
    related: programmers.stackexchange.com/questions/141005/…
    – Doc Brown
    Commented Mar 17, 2014 at 6:33
  • 2
    Implement a style guide/minimanual? I mean, that won't them write better code, but everyone is capable of following guidelines that require to write trivial things in a single particular way.
    – Peteris
    Commented Mar 17, 2014 at 12:40
  • 10
    I already see you having a problem with a lack of teamwork here. You should be educating and nurturing the junior; not just rewriting his or her code and complaining about it.
    – James
    Commented Mar 17, 2014 at 15:35
  • 3
    @Ramhound since Turing devised the Turing Machine I guess. But back to the point, if you are the senior that should get the juniors to hear you out. Teach them how to do the right (or in the convetion) way. But it is important to ask their opinion and maybe they have some ideas for how to do the stuff in a better way. Also if you are using raw CSS for any non-trivial project you are doing it wrong, try to get your project to adopt LESS or SASS.
    – Hoffmann
    Commented Mar 17, 2014 at 17:45

14 Answers 14

79

I believe you are looking at the problem the wrong way - you are missing a great opportunity of teaching the juniors how to write better code.

If you habitually re-write their code, you might give your juniors the impression that you don't value their work, which will lower their morale, and not help them code better the next time.

A better approach, I believe, is to add to your team's development process a code-review task. It doesn't have to be about every piece of committed code, and it doesn't (I would argue that it shouldn't) have to be conducted only by you - whenever a member of your team finishes a big enough task he should pair with one (or more) of his team-mates, explain the code to them, and receive constructive opinion and criticism about his design, coding-style, possible bugs and security issues, etc.

When the code-reviewing team-mate is you they will learn from your expertise much more then when you simply re-write their code (they get a chance to hear the reason the code should be changed), and might take less offense.

Giving them a chance to also conduct code-reviews will further enhance their abilities - seeing how other people write code and why - and will raise their self-esteem.

They will also learn a lot if you give them a chance to review your code. You might learn something too - so don't do it just for show!

9
  • I 100% agree with you, however I wonder how to apply this in the presence of tight deadlines. Do you suggest doing code reviews even though they may suck up more of our precious limited time (in both the reviewing process and the corrections made after the review)?
    – Phil
    Commented Mar 17, 2014 at 14:48
  • 3
    I don't think that a team-member should alter another team-member's work without his co-operation/consent. The person who wrote the code should be responsible for it, and unless the code contains a critical bug and the original writer is unavailable, being on a tight schedule is no reason to change someone else's code. If you find the code too messy - communicate it to the developer, and tell him to clean it up for the next release.
    – Uri Agassi
    Commented Mar 17, 2014 at 14:51
  • 23
    @Phil When calculating a deadline, time for code review sessions should be taken into consideration. It is not an extra step on top of the devleopment process - it is an integral part of the development process.
    – dj18
    Commented Mar 17, 2014 at 16:10
  • 2
    Also, training juniors via code review may have a certain amount of time cost now (which as dj18 says should be factored in to your deadlines and estimates anyway) but it will be repaid in due course many times over as it frees you up to do more original work. If your deadlines are so tight that you never have the chance to do this, it smells rather of a death-spiral... Commented Mar 18, 2014 at 16:54
  • 2
    @JustinPaulson don't get me wrong - the fact that someone wrote some code, does not make this code "his". A week later some other team member will get a task that will require her to modify that code, and she is definitely should change the code for her needs. However, I don't see a use case where someone should 'clean-up' someone else's code for the sake of cleaning up, especially not as a last-minute thing in a tight deadline.
    – Uri Agassi
    Commented Mar 18, 2014 at 21:14
29

I have said this before and will say it again "working code is more valuable than pretty code".

If you change code the chances are high that you will change its behavior, if this is tested code then you have just invalidated all the testing effort, and will need to repeat the tests.

By all means encourage your juniors to write clean understandable code, but if you are going to re-write everything they write then you are wasting your employers money several times over. They have to pay for your juniors, then pay for you to do what they have already paid your juniors to do, and then pay for you once more to do the job they actually hired you for.

12
  • 11
    "if this is tested code then you have just invalidated all the testing effort, and will need to repeat the tests." No, you did not invalidate any testing effort. You'll just have to re-run the tests, which you should do for each commit anyway. If that takes so long that it's considered infeasible the tests are crap and should be fixed.
    – l0b0
    Commented Mar 17, 2014 at 12:41
  • 7
    It should be worth noting that continually writing sloppy code in order to "make it work" will lead to big ball of mud. It's fine if it is an exception, and not the rule. If it becomes the rule, you should talk to your boss that deadlines are not the only thing to consider.
    – Neil
    Commented Mar 17, 2014 at 12:58
  • 20
    The problem is that ugly code quickly devolves into broken code.
    – Telastyn
    Commented Mar 17, 2014 at 13:13
  • 1
    @ramhound - sure, but the OP (and almost everyone else) isn't talking about code that is simply using old standards - they're talking about awkward, inconsistent, shoddy code.
    – Telastyn
    Commented Mar 17, 2014 at 17:17
  • 1
    @JamesAnderson This is an extremely short-sighted perspective. Code is written once but it is maintained for the entire life of the product. Most coding is refactoring. How long are you actually writing down code on a blank screen before you are then tweaking it and seeing if it runs as expected? Therefore you are refactoring, even in the first hour after starting a new class. The cost of refactoring ugly code in subsequent bug fixes and enhancements will outstrip the cost of a little time spent up front with code reviews and moving the team toward one clear standard. Commented Mar 18, 2014 at 18:56
16
  • The short answer is: no. When times are hard, sometimes you just have to put your head down and take the aesthetic bullet. ;)

  • A more pragmatic answer is to time-box it. Budget an hour to run through and clean up one specific aspect of the code. Then check it in and do some real work. But be honest with yourself about keeping it constrained.

  • Sometimes, though, a little bit of clean up makes the work go faster. Even some quick search-and-replace type changes make everything a lot more accessible.

  • Be wary of style wars. Especially in a tight-deadline situation, if you're going to undo some stylistic preferences that the other programmer will just re-do, then again you're better of waiting until you have time to really work out how you want to address those stylistic issues cooperatively. (Which means some give and take.)

But there's a judgment value in the answer. I would say "moderately" important. Clean code really can make the work go faster, and the code quality is, after all, part of the deliverable. I don't think I can touch code (even my own) without spending some time on cleanup. But make sure that fussing with style and format, and style wars, don't become more important than getting the code to production.

9

When fixing code, and having deadline, I use normally two rules:

The code is awful but it's possible to find an issue in reasonable time and fix it

I fix a problem and leave the rest intact.

The code is so messy that it's really hard to find an issue there. Fixing something causes breaks immediately something else. It would be probably faster to write that code from scratch than fixing it.

Then I have no other choice than rewrite/refactor until the code will be clean enough to localize and fix the bug.

The borderline case is:

The code is messy and really bad. It's still possible to fix a bug in reasonable time, but the code structure will make it really hard to maintain. Any new feature is very likely to introduce new bugs or cause significant performance decrease.

In that case, the code is to be fixed, but only when new features are to be implemented, on during the idle time, never in bug-fixing time in face of deadline!

1
  • The problem with the "borderline case" is that other modules tend to spring up that make use of that code. It then becomes very risky to change as other modules may now rely on "incorrect/undesirable" behavior. So you end up stuck with code that is really hard to maintain that makes you cringe each time you see it and makes you want to go elsewhere for employment. At a minimum, bad code needs to be walled off by someone who knows what they are doing. That way, it can be fixed at a later time without incurring as much risk as just leaving it until someone has time to get around to it.
    – Dunk
    Commented Mar 17, 2014 at 21:53
6

I would be interested to know at what point in your process you are finding this problem?

Strictly speaking, in this magical ideal world which none of us inhabit, all code promoted or deployed should be perfect. It isn't so sometimes you have to be pragmatic.

However, if you have a code review process, it should be highlighting this before testing. If you're constantly up against deadlines, are the problems the estimations for delivery meaning that a key component of any development process - ie - code review - is getting strangled?

Your juniors are never going to learn to sit back and absorb better ways of doing things if you don't take the time to make it a part of their dev process to learn. It sounds to me like you are not doing that.

4

Depends on the overall culture. If tight deadlines are sporadic, accept you will have to do cleanup later. If they are constant, than you are structurally building up technical debt and you should take up the problem with management. If they dont address your concerns, better start looking for other job opportunity as company culture would most likely meet with darwinian principles soon.

3

In order to help curb the problem in the future, develop an internal Coding Standards and Practices document that all employees must follow.

For the current batch, clean up the code according to the S&P document as you refactor the code but only when you refactor.

5
  • I've worked for some big, very process oriented companies that were willing to spend oooooodles of money to ensure coding standards and practices were met. THEY NEVER WERE until automated tools started enforcing them.
    – Dunk
    Commented Mar 17, 2014 at 22:00
  • @Dunk Does the US military count as "big and process oriented"? They use S&Ps all the time: stroustrup.com/JSF-AV-rules.pdf
    – Casey
    Commented Mar 17, 2014 at 22:31
  • They most certainly count as the gold standard for coding standards and practices. Every contract requires them. However, as hard as companies try to adhere to their standards and practices, it doesn't happen reliably and consistently. There's just too much going on. That's why automated tools are necessary if you want to include the word "must" in your suggestion, as you did. The DOD recognized the impossibility of adhering to standards via manual means and that's why congress passed a law in 2011 requiring defense contractors to start using automated tools for performing these checks.
    – Dunk
    Commented Mar 19, 2014 at 15:35
  • BTW, I'm not saying that there is no need for coding standards and practices. There absolutely is a need. I just have a contention with the "all employees must follow" part, unless you also mention something about enforcing this via automated tools.
    – Dunk
    Commented Mar 19, 2014 at 15:50
  • @Dunk The JSF-AV team must have recognized this, the document specifically mentions the use of automated tools as a way to enforce the S&Ps (back in 2005)
    – Casey
    Commented Mar 19, 2014 at 19:01
2

I'm fairly inexperienced with programming. As a student, however, I often commit to peer review and partnerships on projects. If there's ample time to finish a project, I'll go ahead and clean up a team member's code for clarity and readability. More often than not, I'll find it difficult to even sift through the first 100 lines or so. In these cases, I'm more than willing to extend a hand to assist with teaching a fellow programmer better habits and coding. If there just isn't enough time, I simply copy/paste, and work my projects into the big picture dealing with their poor interfaces. Afterwards, I'm sure to offer plenty of advice on coding technique. When it comes to peer review, constructive criticism (regardless of how unwelcome) only benefits both he/she and myself in the long run. If I collaborate with the same individuals in the future, I can rest assured knowing they had some informative guidelines pressed into their minds, and only hope that the next go round runs much smoother.

Overall, if you have the time to spare, take it to teach your newcomers how to conduct their work so that everyone is beneficial. Take a minute and teach them what has worked for you, and what hasn't. If you don't have the time, bare with their work for now and be sure to get back to them when you have the chance. Let them know that there are better ways of doing things, especially if you'll be working with them in the future.

2

Improving overall quality is vastly superior to using a single person as a "filter" for a larger group. On that note:

  • Pair programming works like a souped-up version of code review for understanding how to develop - it's like the difference between reading and doing, telling and showing. Watching code evolve and quickly discussing changes is immensely helpful to understanding not just the how but the why of refactoring and good code. In my experience it's faster than developing alone, since ideas get tossed around continuously, ending with an overall higher quality result and better understanding of both the code and the other person's thinking.
  • Linting tools can verify that coding style is being followed. This teaches everyone how to format code, and errors should drop off quickly once developers remember the standard.
    • Make these part of the build process to ensure that it's fixed before committing.
    • Use language templates to ensure that your CSS, HTML, JavaScript and server-side code can be checked separately.
  • Validation tools can check that the generated output is sane. These should also be part of the build process.
2

The best practice would be to have a coding style guide, and have regular reviews, so that when you're approaching a deadline, you're not faced with this problem.

My recommendation is for you to show leadership, and spearhead the regular code review. Management doesn't get pushed from the top to ensure that regular code reviews happen, but my experience is that they will be impressed when a programmer steps up to schedule and hold regular code reviews.

There are many benefits for your people, who will:

  • learn better style
  • follow better practices
  • learn to research what they're doing

And some benefits for yourself, you will be:

  • more efficient during last-minute debugs (which will always happen)
  • recognized as both an expert and a leader by both your team and management
6
  • 1
    The problem with coding style guides is that they tend to turn into books. Most people are willing to learn and follow a fairly modest set of rules. Unfortunately, at some point these guides always grow beyond people's ability to learn and remember all the rules. You need a tool that will do style checks automatically, period. Code reviews should not be for performing grammar checks, they should be for finding errors and misunderstandings.
    – Dunk
    Commented Mar 17, 2014 at 21:57
  • As a Python programmer and leader of code review, I've printed off PEP 8 and Google's Python Style guide at least a dozen times to be passed around. Whatever programmers won't learn from them will find themselves falling behind those who do. That said, I agree that a style checker is also a good practice if you can implement it.
    – Aaron Hall
    Commented Mar 17, 2014 at 22:02
  • I don't use Python, so I don't know the tools that are available, but if you are relying on code reviews to enforce your style rules then you are wasting hundreds (if not thousands) of hours per year for something that you could get done for you for essentially no time cost. I certainly wouldn't go ahead and implement a home grown version. I'd spend the money to buy a commercial version that will be WAY better than anything that can be home-built in off time. Even the expensive tools will pay for itself many times over.
    – Dunk
    Commented Mar 17, 2014 at 22:09
  • Python, being the open-source cornucopia that it is, has all kinds of free tools (pylint, pep8, pyflakes), some of which we've combined and improved, which since we have thousands of developers, really scales nicely.
    – Aaron Hall
    Commented Mar 17, 2014 at 22:19
  • 1
    :I was referring to your "if you can implement it" snippet. If you could buy a style checker then that's the way to go. If you could have your team implement something as useful as this in a reasonable time then there has to be a company/open source that has already done it. So it would be far more cost effective to simply buy it. I'm sure it would be better and more up to date than a "non-productized" home grown version. If you have thousands of developers then I vastly underestimated the amount of savings an automated style/security checking tool would provide.
    – Dunk
    Commented Mar 19, 2014 at 15:45
2

I can see the reason in the "don't fix what's working" and "don't waste your time on what's not important to the client" answers. PMs are worried about risks and this is fine.

Also I understand most people don't take this kind of fix well. I understand this too.

Said that, I believe that most deadlines are artificial. Real systems live ever more than the deadlines and the bad design you do today will fight you back forever and ever. People run to deliver something in a few months and spend years after this fixing some bad decisions in a code that is being run in production.

Tech debt is the word. It will come back someday and someone will pay for it.

So IMO, I think you are just right fixing the broken design, and being professional (specially for the juniors) also means that you must know how to take criticism and how to learn from it, even if it's not polite. In fact, most of life is not polite anyway.

0
0

Any straight answer is going to be extreme. Clearly there are cases in which the deadline is so tight that you must use ugly code, and there are cases where the code is so ugly that it's worth missing the deadline to improve it. What you need is methods to judge which you're in, and perhaps methods to set realistic deadlines that allow time to write better code.

Don't save the cleanup for later. Unless you habitually have periods with nothing to do but refactor, there is no "later" in which it will somehow become higher priority to tidy up the code than it is right now. The routine is "red, green, refactor", not "red, green, do something completely different for two weeks, refactor". Realistically you will not change the code until next time you're revisiting it for some other reason, and you'll probably be on a deadline then too. Your real options are to fix it now or leave it.

Of course well-styled code is better than badly-styled code, assuming you plan to ever read it again. If you plan never to read it again, then do not tidy it up. Ship the first thing that passes the tests. But that's a pretty rare scenario, for most programmers it happens approximately never. Ignoring that case, only you have the details of your real case to make a judgement how much it costs to fix vs. how much it costs (in increased future maintenance) to not fix it.

There are certain things that are no harder to fix at the point where the code requires maintenance, than they are to fix now. These don't actually benefit you much to fix now. The most obvious are trivial to fix (whitespace errors and the like) and so it's difficult to imagine that you have time to ask this question but not to fix them ;-) For that ones that are not trivial and are of this kind then OK, you have some code that isn't ideal but you must be pragmatic. It works and you're on a deadline. Use it.

There are certain things that are considerably easier to fix now than they will be later when (a) they're not so fresh in everyone's minds; (b) other things have been written that rely on them or imitate them. These are much more valuable to fix now, so prioritise them. If you don't have time in your deadlines to fix these, then you need to push as hard as you can for longer deadlines, because you are building up debt in your code base that you'll probably have to pay next time you visit the code.

The preferred method of fixing code is through a review process. Comment on the problems you have with it, and send it back to the junior to change. You might give examples of what you mean and leave the junior to find all the cases in the code that they apply to, but don't just finish their code for them. If you do then you give them no means to improve.

You should write common problems up into a style guide that says "don't do this, do this instead", and explains why. Ultimately the reason is allowed to be, "in order to make our code aesthetically consistent", but if you aren't prepared to write down your rules with some justification then you probably shouldn't be enforcing them either. Just leave each programmer free to choose.

Finally, beware of the tendency to tweak stuff indefinitely. The returns diminish, and you need to learn through experience where they're still good. It is absolutely essential that you form a realistic idea of what is good enough, or else you can't have that negotiation in which you make sure that your deadlines give you time to create "good enough" code. Spend your time on things that are not good enough.

0

As so many have said before, whatever you throw in the air will always come back down. I believe in strong uniformity across a code base. Of course some things really do not matter that much. Naming conventions on local variables within a procedure for example. However for anything structural, it should be fixed right away, before the final merge into the main trunk. It may only be slightly shoddy when you look at the individual procedure or class, but if everyone commits "slightly ugly" code, it really fast becomes really ugly as a whole.

Ugly code that works often works fine 90% of the time but falls apart at the edges. Making sure it doesn't is usually simple enough by following only a few simple rules. First, it should be mandatory for every programmer to define and document exact constraints for every procedure or functional block they produce.

Second, for every procedure there should be a test against those constraints. This should be a simple unit test the programmer can (and has to) run locally against his procedure before committing. Obviously this is easier to manage with a proper testing suite, but even without a test should be written, and possibly committed in a partial class that can be excluded from the build.

Thirdly, a set standardized development environments with pre-configured tools is invaluable. A TS server is superb for this. Everyone has the same exact tools (and versions), the same configurations, and the same updates. Install a refactoring tool like CodeRush or Resharper, preconfigured to your standards, and instruct you programmers that you will reject any commits that still have warnings. Now you can use your team's code review time to actually improve on your rule set from their feedback, and your team happily correct themselves without you having to constantly clean up afterwards. It is also much easier for a programmer to take code criticism from a properly configured tool than from a colleague or boss, where the standards may seem arbitrarily defined or are not properly understood. If the IDE tells you that your code is shoddy, no one will argue with it and it will be corrected. You will find that code quality will rise dramatically, and the team as a whole will spend MUCH less time refactoring and cleaning up after a few weeks. Programmers will also get used to the rules and stop writing crap code.

Lastly, the simple fix here is simply to give the programmers an incentive to improve. Programmers are by definition competitive. Everyone wants to have the nicest or fastest code. A good way to motivate everyone, improve on productivity, and root out the incompetent is to calculate a weekly weighted score board for everyone, taking off points for rejected commits and broken deadlines for example. Show the top N at the weekly team meeting, maybe even pay lunch to whoever is first in the month's averages.

0

I suggest using a review tool. If you have a Git based repository you can use the Gerrit review tool. After a few rejected commits, the team will learn the standards you want to follow and future commits will not require any additional work from you.

Commits will wait for your acceptance. If you see any lines which should be rewritten, you can write comments and your teammates can fix the code on their own based on your requirements. It's really a good way to learn team members coding standards.