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.
- 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.
%operator is more debatable. Arguably, the old code there was easier to understand, particularly for junior developers who may not use that operator often (except for FizzBuzz). Do you think he would have complained so vehemently if you had only split the lines? That said, his reaction was very poor. Be careful with this guy in the future...