13

I am currently mentoring a co-worker who refuses to delete code. He currently works as strictly an html / css developer, but when he edits CSS, he does not delete code. He will comment out anything that he wants to change, and apply his changes.

Here is an example of what I mean: When asked to remove padding from an element, and change color to red: 

.sampleRule {
   /* padding:20px */
   margin-left:20px;
   color: /*black*/ red;
}

When I asked him about this behavior - he says that he does it for documenting purposes. He wants to know what the properties were before the change was made. He has a designer background so I can understand his thinking, but our code base is littered with code that looks like this.

I suggested that he uses source control if he is curious about viewing past changes (you can use TFS, for example to show all changes done to a file).

I am currently code reviewing all of his work, but I'm not sure how to approach this situation. Should I speak to my boss? Should I delete this behavior when I see it? Should I leave it alone?

Thanks for the help.

LennieCodes
  • 241
  • 2
  • 6
  • 3
    Yes, our company uses source control. We check in all changes so he can just use that but he refuses to right now. – LennieCodes Jul 14 '17 at 17:49
  • 2
    Asking how problematic this behaviour is and whether it's severe enough to approach your boss about or whether you should just ignore it is not a question appropriate for this site. This is not a programming site, we are not here to make judgements about the appropriateness of coding habits. If you believe that this behaviour is unacceptable (and/or it's clearly disallowed according to your coding standards) and something needs to be done, the answer simple enough: talk to your boss. – Bernhard Barker Jul 14 '17 at 17:59
  • Possible duplicate of How to avoid bad practices of work by employees – mcknz Jul 14 '17 at 19:16
  • @Emotive.io, you have now identified 2 issues - leaves commented out code in place and chooses not to use a version control tool. The second justifies the first. I can't believe Devs are not introduced to version control in the first class of any CS course. Explain the benefits of version control, private branching, etc. (and checking in changes w/useful check-in comments) and how that makes his performance better. Why/when did he make that change? See check-in reason and date. Done and there for all to see, unless on a private branch. – Ian W Jul 14 '17 at 23:24
  • How would he remove that color: /*black*/ red; declaration, since CSS comments aren't nestable? – typo Jul 14 '17 at 23:25
  • To put your foot down about it you need to have it as a standard, not just your opinion, and I would recommend it be your standard. Once you have that, then he needs to understand the concept of mentoring is to learn, not ignore. – dlb Jul 15 '17 at 01:07

4 Answers4

24

From what I am gathering, he is using source control like everyone else, but rather than just deleting the old code once he is done, he likes to keep all of the old code as comments.

This needs to be addressed in a company policy. If your company policy states that all old code should be removed so that files remain as small as possible, that's the policy he should follow.

Yes, you should talk to your boss to see what the policy of the company is. No, I wouldn't delete any of his work.

If there is no policy and this is just your personal feeling, then it seems like you should just agree to disagree. Just because you feel this is the correct way to code, doesn't mean that everyone would agree with you.

curt1893
  • 4,576
  • 2
  • 21
  • 29
  • 9
    If there is no policy, that doesn't necessarily make it "just a personal feeling". If you feel it affects the quality of your deliverables, talk to your boss about that and see if you can help establish a policy. When you do, take care the conversation is not about personal taste or you-vs-co-worker. Focus on the quality and maintainability of your product. Make sure the new policy includes a solution to meet your co-worker's need for documentation. – marcvangend Jul 14 '17 at 21:24
  • 7
    You should be flagging this in the Peer Review, and not accepting it until it is fixed. One of the many purposes of a code review system is to build and establish standards across repositories. As a second opinion, when there is a source control system in place any commented out code is fair game for deletion. – Bill Leeper Jul 14 '17 at 21:48
  • your team needs a working agreement that outlines things like this as well - everyone abides by the agreement and don't leave it up to a 'feeling' – NKCampbell Jul 14 '17 at 21:52
  • 1
    Uh, no company policy is ever going to be able to address everything and using common sense should be required. This "code style" is weird, I've never seen it before, and any competent programmer ought to universally agree that it's not useful. Source control is for tracking history, not comments. – Matti Virkkunen Jul 15 '17 at 00:16
6

This is actually a fairly common practice and the "rightness" or wrongness of this varies wildly according to shop standards.

If there are no shop standards prohibiting this, then there's nothing wrong, if there are, he's violating procedure.

Ask your boss what the shop standards are for this and act accordingly.

If you feel these should be shop standards and are not, bring it to your boss's attention and make your case to have it as an established standard to delete code rather than to comment it out.

Old_Lamplighter
  • 159,693
  • 108
  • 436
  • 585
  • 6
    Whether or not there are any "shop standards" and regardless if it's a "fairly common practice", it is generally a bad practice. Simply commenting out the old value does not tell you WHY it changed, which is often more important after the fact than what the old value was. – Ian W Jul 14 '17 at 23:17
  • 3
    Also it doesn't help to understand which parts of a file changed at a given moment. You just know what was there before but nothing more. With source control instead you know exactly how the code mutated over time. – Fez Vrasta Jul 15 '17 at 08:53
5

Preface: I have been in the field for 11+ years, and have been leading front end dev teams for 3+ years a large companies such as Nike.

I disagree with the previous answers on a few of their points. (This is assuming based on the fact that you said you're mentoring him that you're also his lead, if not and by boss you mean the lead developer than listen to the previous answers and go talk to them, if by boss you mean a owner, department head or some other non developer then continue reading)

Firstly: I would NOT go to your boss. You should view going to your boss as a nuclear option to be reserved as an absolute last resort. You boss had confidence in your ability to mentor this dev and handle things like this. Every time you come to your boss to have him handle something like this it is going to erode a little bit of the confidence away. I assure you your boss has much better things to do than dictate code standards. Only involve your boss if you need clarification on something or things need to be escalated to the point of a formal reprimand or dismissal.

Secondly: this isn't a company policy decision. Code formatting and code standards are entirely at the discretion of the lead developer. You boss does not care or need to care about nuances like formatting and comments.

Thirdly: This is not common, In 11 + years I have only ever ran into one company that would allow leaving comments in code for history. The only places I would expect to see this type of coding is a outdated shop that ignore best practices and has no source control, PRs or GIT repos, and if that's the case, RUN run far far away and don't ever look back.

If I were you I'd first give him a tour of GIT and show him how it works since he clearly doesn't understand commit history. Hopefully that helps ease his concerns and makes him understand where you're coming from when you say not to do it and that will be the end of it.

Second I would refuse to approve any of his PRs until he follows your suggestions, you're the lead, so lead. If the code doesn't meet your standards then it shouldn't be merged, it's that simple.

If he still refuses to change it and lets his PRs sit out forever with you blocking it then it's time to talk with your boss about the dev. Maybe have the boss tell him to listen to you, or write him up or possibly even dismissal depending on how he reacts and his overall performance.

You're his mentor and if he's refusing to listen to your mentoring then he's not doing his job and he's preventing you from doing an important part of yours. Keep in mind if you're boss feels like this dev isn't learning anything or listening to you and you haven't said anything to your boss then that reflects badly on you not the dev your mentoring.

Lastly if your company doesn't use GIT or PRs (pull requests) then that is a much bigger problem than the dev leaving comments in and you should be pushing to get the company to follow modern best practices that prevent problems like this or start running away and looking for a job that will be more beneficial to your career.

efarley
  • 151
  • 3
2

In many languages this is a pretty good practice, at least temporarily, as it allows changes to be easily reverted and others in the codebase can see the recent progression and changes that occurred. However, it is a bit strange to do so in CSS where changes are pretty minute and easy to swap around and it is usually easy to immediately see changes.

Despite that, It's not super uncommon, but what would be uncommon is leaving the code comments in after awhile. I would just tell him that it's fine but after the changes are made and he decides they're good to stay, go back and remove the comments. There's really no reason they need to be there if the changes are accepted and expected to stay.

Also it might be a good idea to add a tool to minify the files and remove comments before deployment, this way it doesn't matter in a technical sense if the files are larger or his comments remain. It's not a remedy to the source of the issue but a helpful measure to prevent its effects.

Ghosts
  • 29
  • 1
  • Definitely the whole benefit of flagging recent changes is completely lost if the code is covered in these flags. – Myles Jul 14 '17 at 19:44
  • 6
    I can't imagine that this is a good thing to do considering things like git will cover the case entirely and 10000% more elegantly. Does he keep comments after a second / third change? It's enough to make my eyes bleed just thinking about trying to maintain such a thing. "I'm just going to Ctrl + F where this style is appllied... 1000000 commented results.." – easymoden00b Jul 14 '17 at 19:55
  • It's OK to comment out code while you're working on it, but not when you commit the changes. All change control systems provide better methods to do all the things you listed. Temporary is fine, but it becomes permanent when you commit it. – Robin Bennett Mar 26 '19 at 15:10