Code readability, conventions and should I let go

Question

While chasing a bug, I found a few lines of code I found hard to read and improved them for readability.

This change brought the code closer to what our Coding Convention says, which also states that we should "correct code that isn't written using this style".

Now one of my peers, who has seniority let's say because of the length of his tenure there. Smart guy, devoted to the code, and has written important pieces. Pulls me aside and says:

Why are you doing this? You are changing my code. We are a team. This code has been written in such a way to be meant to be understood from anyone on the team. And has to be maintainable even after 100 years. If you don’t follow the team conventions we have to part ways, or I will talk to managers so you never have to touch that code again. This thing ... will not be understood by junior developers, and I know what you’re doing there, just because I can see the diff. I written this code and know every line in it, and now it is hard to follow it. It is not readable and I write my code so it does make clear what I think. You should do what I tell you to do – this means don’t touch my code, and if there are bugs just fix the bug and that’s it.

OK, I admit I am guilty I didn’t follow the Code Convention to a T, but generally I think my version is more readable than the version before.

I reversed the changes, and just fixed the bug which was a few lines below. But this leaves me uneasy as I am complicit in leaving messy code. It is not the only place in the code base that has long lines and multiple statements on one line, but at least I didn’t have to spend so many iterations with it.

Should I let go, or go and talk to higher ups, his managers, their managers, etc. On one hand for 3 lines it will be stupid, given the fact I recently joined so they don’t know me that well, on the other hand it is a matter of principles and this bugs me a lot.

Clarifications:

• The person I had this discussion with is not my manager, he is a peer. We are on the same level. His perceived seniority is based on the fact he's been there longer than me.
• The code convention clearly say that I can change code to make it in the spirit of the code convention. The convention was written by his direct manager.
• @Fattie - I tend to agree in general that both versions of the code are garbage. I am fan of Uncle Bob, Grady Booch, Kent Back, Martin Fowler, etc... So I know what you are implying. Now I didn't go all the way to make it unrecognizable, as I was afraid just of getting into this territorial dispute. I merely followed the convention in this case.
• I've got the impression that some of you assume that being new at that place - means unexperienced. Please, don’t assume that.
• To me code readability is precursor for easier maintenance down the road. I know I can cut corners and leave it as it is. I am going to change ship eventually. Future devs will have to deal with it. Don't need to complicate my life for 3 lines of code. Just wanted to collect opinions if the fight is worth it. In that regard (for this case) the answer by @A.O makes the most sense.

Answer 1

Let it go. You are trying to compromise among 3 or 4 priorities: your preferences, the Code Conventions, the preferences of the senior dev whose code you changed, and the team's ability to make sense of the code. According to the senior dev, his coding style makes sense to the team. So priorities 3 and 4 can be combined. Now it's between you, the code conventions, and senior dev+team.

Between these options, the team's conventions, and your relationship with the senior dev, in my opinion, matter more to the team's ability to get work done, than your preference and the code conventions.

Factoring in that you are new, I would recommend to let it go and learn from it that what you think is right is not necessary what always should end up happening.

I suspect others here might push back and there is definitely room for different ordering of priorities, but this is probably the path I would have taken, at least while still a relatively new employee. Good luck!

Answer 2

So your senior and you disagree on what the coding conventions means and which way is more readable. The way to resolve this is to clarify your understanding so everyone is one the same page.

Therefore, I'd have reacted as follows:

S: Why are you doing this? You are changing my code. We are a team ...

J: I tried to make it adhere to the coding convention. It says we should do XXX, and improve existing code when we see it?

And then listen to what he says and genuinely try to understand his point of view.

And once the technical debate is over (and tempers have hopefully cooled down), I'd address the social side of his feedback. Again, this best works as a question:

What should I do if I see something in your code I don't understand or think could be done simpler?

And again listen what he wants.

This accomplishes several things:

1. His rather emotional response indicates that he is emotionally invested in his code and feels his expert judgment disrespected. By asking for his expertise, you show that you value his expertise, which should sooth the discussion.
2. It communicates you did not mean to disrespect him or flaunt social norms. Rather, it makes him realize that you simply didn't know yet what was expected of you, and that you are trying to fix that.
3. It clarifies what is expected of you to avoid further accidental conflict.

Answer 3

You've already reversed the changes. So let it go.

That being said, the next time this happens. Be sure that there is no wiggle room with your interpretation of the coding conventions. Because, if you're going to have a battle with someone, be sure to pick a battle that you have 100% chance of winning.

This is important especially if you do not have as much social capital with management as this person has already.

Also, be sure to add unit tests before you modify too much of his code. If you introduce bugs to his code, you will never hear the end of it. And when you do find something of his that is worthwhile for you to change. Then, go ahead, but be careful and follow the official conventions (and if not the official conventions don't address a particular issue, follow the Code Complete conventions at least).

If he does complain to you, then you can just tell him to fight on changing the code conventions, and that once he receives approval for those new coding conventions he has in mind, that you will be glad to take a look at them and implement them, but not before then.

Also, you should probably ask management to organize weekly code reviews. After all, if you have a chance to discuss coding decisions during code reviews. It will give you a safer and less confrontational way to understand and/or challenge some of your peers bad habits.

Answer 4

A lot is going on

There are quite a few things going on here. Many of these have been addressed, and I will briefly mention them and my vision of what you should or shouldn't do about them. However, there's one point I haven't truly seen addressed yet which I take to be the biggest problem and I will address that in more detail.

Bugs & style fixes

You fixed the problem while at the same time changing the code.

In my opinion, this isn't a big issue, but it is generally better to keep separate things in separate commits. This would have had the added of separating the conflict from the bug fix here as well.

Differing views

You disagree about what version follows the conventions better, is more readable and is more maintainable.

If he's your senior, it might be good to follow his lead on this. Alternatively, it might be good to have someone else from the company have a look at the code together with both of you, looking at it from those three perspectives.

Tough code

The code was hard to understand to begin with.

I've found it to be useful to ask questions about the code in such situations, especially if you know who wrote the code (and/or the code was written recently). Asking questions can both lead you to understand the code better and help the other person see why the code was hard to understand to begin with. This often brings up ways the code can be made clearer all by itself, which you can then discuss before implementing them.

The attack

The other person gets pretty aggressive by suggesting that you have to part ways and that they are going to your manager to keep you off certain code.

As written, this is pretty serious. It depends a bit on which way the difference between the actual things he said and the way you paraphrased him here sways, but purely by the way you have written it, this alone should be reason to talk to your manager. If you do, don't make it about the code, but about his attitude towards you where he seems to consider making threats to you okay.

His code

He doesn't want you to edit his code.

In my opinion, this is the biggest problem. This is a pretty toxic attitude and I think it strikes at the core of the issue. Hobbyist programmers are often very protective of their code and sometimes that can be seen with professional programmers that have worked on code by themselves as well. However, it does not have a place in any professional setting and is extremely toxic when you're responsible for the code as a team.

Ultimately, the problem is that the code isn't his. The code is the company's or perhaps the team's, but it's not his. Instead, it is code he has written. I think it's important not to refer to it as his code, but as code he has written. Without stating it isn't his code or correcting him when he says it's his code, it can be beneficial to consistently refer to it as code he wrote yourself. This goes both for the moment you are discussing the situation with him, as well as when discussing the situation with other team members or a manager.

First of all, I'd say that I would let this incident be what it is for now. However, I would also not go out of the way to avoid touching code that he has written in the future. It sounds like this is going to be a thing that is going to come up again and walking on egg shells all day isn't going to be of help to anyone.

Assuming you're in a team with more members than the two of you, the first thing you could to is talk to other team members about the issue. Ask them if they have experienced the same behavior and how they handled or would handle it. Your coworkers are a resource that shouldn't be underestimated. If there are no other members on your team, you can instead ask other colleagues that have had to work with this person in the past.

Then, if this programmer turns out to be overprotective of the code he wrote on another occasion, look to apply the suggestions from your colleagues. Beyond that, you can also ask him if he is upset because you changed code he wrote or that it's that he thinks the change affects the code negatively. These are two completely separate issues and it's important to make him aware that they are.

After that point, it seems like it would be time to escalate the issue. Talk to your team lead or manager (whichever is more appropriate) and explain to them that this person seems to take issue with you touching the code they wrote. While you are open for discussion about the quality of any code (which you should be), it seems that it's seems to be the fact that this person wants the code that he wrote not to be touched (by you - if that's what your impression is). Explain how this falsely partitions ownership, responsibility and understanding of the code and how that's detrimental to the product and increases the way the company relies on specific people. (Of course, such an explanation shouldn't need to be nearly as detailed when talking to a team lead as when talking to a manager.)

In conclusion

So, essentially, I think that the most important thing is to separate out the different issues and tackle them individually. This should also make a it a lot easier to deal with each one of them, as you now know what's going on.

Answer 5

Saw a number of answers saying "Let it go". I can't agree with that, because the issue is going to come up over and over again.

A team needs standards, specifically to avoid arguments like this. I'd say to talk to your co-worker again, once tempers have died down a little. Ask him to review the coding standards with you, and together work out -- if possible -- how you guys think the code should be organized. If y'all can come to an agreement, coolio. If not, recognize without rancor that you're just not on the same page, and get manager to help you two resolve the issue.

You deleted your specific example (which I thought was too bad, it offers some context), but let me just add one thing... That one potentially tricky part (incrementing index modulo count to wrap back around to 0) should be understandable to J Random Programmer, but it wouldn't hurt to drop in a quickie comment on top of the line as a reminder... ;D

Answer 6

I don't think you can let this go: your colleague is very protective of the code he has written. So every time you change a piece of code he has written and does not have a bug, he is very likely to come to 'talk' to you about it.

And your colleague is trying to bully you, make no mistake about that.

First off, the code you are both working on is not his nor yours. It is owned by the one paying you.

Second, you should not do what he says you should do, unless he is your boss, what he is not. You should do what your employer wants you to do.

Third, I agree with you that you should leave things better then what you have found, so making the code more readable is good thing.

---

So, you don't want to complicate your life for 3 lines of code. Good on you! But what are you going to do the next time this happens?

I suggest to get your team lead / manager involved next time this happens. See points, one, two and three.

Answer 7

While your "senior" peer may have overreacted, you should also consider the possibility that he was right in a way.

Unfortunately, the edits removed the code itself: maybe this is typically better for most non-IT users, but in this case an important piece of information was lost.

The way I see it, you've improved the readability of code by most formal metrics: the conditions are more logically organized, there are braces and stuff. However, your introduction of the % operator (integer remainder) is a Clever Trick that will confuse 90% of junior developers, and apparently some seniors as well. Now, overly verbose code is bad, redundant checks are bad, but Clever Tricks are just horrible, unless you are sure that all developers are at least as clever as you are. And you've already got the proof that they are not.

Also, while your company documents say that you should "correct code that isn't written using this style", it would probably be better to mention such changes to the original author, if he or she is available. A quick message along the lines of "Look, I had problems debugging this code, don't you think it's more readable this way?" could have saved you both some embarassment.

Answer 8

When you are new it is usually a good idea to take some time to observe the dynamics and get a feel for the team, etc before taking it upon yourself to make any big changes. A good rule of thumb is "If it ain't broke, dont fix it", or a corollary to that, "If it was broken, someone would have fixed it by now." If the code in question was a big issue, it probably would (or at least should have been fixed by now. It is possible that some code you come across at this job will need fixing, but wait until you are more familiar with the group's system before you decide if any such code qualifies.

Despite how much effort goes into developing universal coding/documentation standards, EVERY WORKPLACE WILL HAVE THEIR OWN STANDARDS and it takes time to infer what exactly what those are.

Answer 9

You might let this incident go, but you shouldn't feel in the wrong. You just have to pick your battles.

Your mentality is absolutely correct. Whenever we revisit old code, if it is not easy to maintain we must first comprehend. Once that comprehension has been achieved, if we don't document or ease the route to that comprehension, we're wasting the time of further maintainers down the road much like the original programmer has kicked that cost down to you. Therefore its the duty of any programmer worth their salt to either document or improve code as you go (the latter is less risky when you have a robust and exhaustive test suite that ensures refactors and improvements don't break functionality). Leaving comments is not ideal, but it does leave a smell that the code needs some love in the future.

Your team is also taking the correct approach to coding conventions. Conventions are not rules, so there must be some leeway to breaking them when it makes sense to do so. But they do represent an ideal that may not have always been achieved and it's more practical to enforce code conventions on new and updated code rather than exhaustively refactoring. In an ideal project, the code should never look like any specific individual wrote it. This enables all developers to check their ego at the door and treat the code as the team's code, worry about code quality rather than who wrote what.

This brings us to your colleague. Their behaviour is not unusual as you might think as programmers do have a tendency to feel pride and ownership over their code (or look to blame others, sometimes, only to see themselves in the history) and can develop ego despite efforts to resist doing so. Others do not resist at all. "My code" is a bad attitude to have, all code should belong to the team. There is an inconsistency between his claim that he's coded things to be understandable by the team if it is in contravention of the team's coding guidelines. He makes it clear that it's coded to be understandable by him first, not the team, and he's upset that you've ruined his comprehension by having the audacity to change things (all good code should be written to be easy to maintain and delete, not be immortal).

It gets worse when he claims that you can never understand his code and threatens to use his influence to put you out of your job. That alone is massively unprofessional, but in response to a little formatting is hugely blown out of proportion.

You do need to pick your battles and I'd let the formatting side of things go. If you're new on the team and someone says that you're not meeting the code convention, all you can really do is defer to their experience and suggest you update the documentation if it has become misleading. If this causes friction, then you need to appeal to your boss as keeping one standard on paper and another in practice (especially at the behest of individual developers) is discordant and undermining.

However, I'd be far more likely to focus on the threat levelled at you. If it was done verbally, you don't have good evidence at this point and you have no good option to act at this point, but I'd be very careful around him in future and try to get everything he says in writing, e.g. interact by email as much as possible. If he threatens you again in writing, you can start to talk to your boss about it. From the sounds of things, his ego is fragile enough that you're going to run into problems with him again, sooner or later.

Where you might run into problems is that you don't know the lay of the land. If this guy really is as bad as he seems, it's possible that large swathes of code have been written by him and have become unmaintainable by anyone but him. As such, he's effectively made himself unfirable as the company can't afford to lose him, which has enabled his poor behaviour. This can be good in the short-medium term for job security, but long-term it's degenerate, counter-productive, and can ruin your reputation. Don't aspire to this.

With that in mind, be careful you're not putting the company in a position to choose as they will put the interests of the business first. Treat your current job as a learning experience in dealing with difficult people. Spend time with your colleagues and work out how they deal with this guy and don't pass up the opportunity to learn other stuff from them too. In the meantime, keep an eye on other opportunities and be ready to land on your feet in case he does have the clout and does kick you out for some other perceived infraction.

Answer 10

The answer to this strongly depends on what is the general approach to "process adherence" within the company, and you should have already a feeling about this.

I have worked in a company which was, on paper, certified ISO9001, but the CEO always bugged us with quotation requests made by quick and incomplete phone calls, though we had a request form where all the info could be collected. You probably guess that in that case enforcing standard was a lost battle.

You have two alternatives:

1. The company has a culture of process adherence: you are right to follow the company Coding Convention, and if the thing escalate, just make clear you followed the standard.
2. Process adherence is just on paper: forget about the manual, and follow the "hint" of this senior.

Answer 11

"The code convention clearly say that I can change code to make it in the spirit of the code convention. The convention was written by his direct manager."

I can't help thinking this is a terrible directive. By all means generate a ticket for "improving" a specific section of code, or bringing it up to standard, but "change it if you don't like how it looks" is a recipe for disaster. How do you control these changes when they're made as part of another fix? How do you test the changes?

Answer 12

As far as I see your problem with your peers code is the formal code convention of which you have a different understanding then the senior.

There are lots of tools out there which will automatically format the code for you by a set of configurable rules. They don't leave much space for interpreting the conventions.

So in a regular team meeting (a scrum-retrospective preferably) you might refer to your discussion with the senior an that you feel unhappy with the situation and then suggest that your team (or at least you) starts using such an auto formatter and nominate the senior to configure the rule set.

Next time you change the seniors code (to fix a bug) the reformatting is done by a tool that he configured. I'm curious how he can put the blame on you then...