[joshstrange]

One of the most frustrating things I’ve dealt with is a junior developer (who, based on years in the industry, thought they were a senior) who would write clever/complex code just for the sake of it. That and touching 10+ things in their commits that were outside the scope of the ticket (“cleaning up other code”).

I’ll fully admit I used to be that person but some people never seem to grow out of it. They see what they perceive as “gross” code, repetitive code, or verbose code and make it their mission to DRY it out or rewrite it in a shorter, more clever way. Cleverness in code should be avoided at almost all costs, code golfing might be a fun exercise but has no business in real codebases, and trying to abstract and centralize logic with only 1-2 implementations is foolhardy at best.

I’ll take verbose, understandable code every day over code that I have to stop and think about for any amount of time.

[Kailhus]

Junior dev here, I’ll bite. I’m not sure of the specifics but I sincerely hope we don’t work together and you actually raised this with them as this fills me with zero confidence in my work, we all have different views on what’s “going too far” regardless of level and as others have said it’s rather difficult to know without being told.

Yes, if it’s in my logic path, I will replace an old regex in favour for a newer in-built function that is much easier to read and I’m certain does the same thing, rename a constant which make zero sense or split the same damn check repeated 10x times into it’s own better named constant or helper function - as long as it doesn’t touch the business logic, who cares! But, I do think it’s important to check the temp with your lead and clear this is an acceptable thing to “tidy up” behind you.

[joshstrange]

> I sincerely hope we don’t work together and you actually raised this with them as this fills me with zero confidence in my work

I'm not sure why you hope we don't work together. I assure you I raised this point with this developer multiple times and it's not my goal to belittle or discourage junior developers. I actually really enjoy mentoring junior developers as long as they are willing to consider they may not know everything and have an ounce of humility towards the codebase they are walking into. Heck, when I come onto a team as a senior developer I work hard to not make changes even if I think I know better until I get a good feel for the code. Too often a junior comes in and wants to change everything to suit their style or their "I reckon"'s, that level of ego grates on my nerves (no matter what their skill level is). To think you can walk into an established codebase and start making changes without knowing why something was done a certain way is not ok. See also: Chesterton's fence [0].

I'll try to address your points one by one for what I think is the right path. My path is not necessarily right for everyone in every circumstance but it comes from writing code for over 20 years.

> I will replace an old regex in favour for a newer in-built function that is much easier to read and I’m certain does the same thing

This is iffy for me. If the old regex is working then I'm not super interested in changing it just to change it. I want to preserve the git history, the commit message, and the linked ticket. Yes, you can step backwards to find this but it's much harder and rarely do I even step back further commits unless I really need the information. Enough times I've annotated a file to see " " => "\t" as the change and now I have to inspect that full diff and annotate the "before" version to see who wrote it and why originally. I understand wanting to change /myString$/ to .endWith('myString') (or similar) but unless you are already changing the logic in the regex I probably wouldn't touch it.

> rename a constant which make zero sense

This I understand and will occasionally do if the meaning has completely changed but changing something "deleted" to "isDeleted" or stylistic renamings are a no-no in my book. It's almost always better to leave a detailed comment on the definition of the constant to help people reading it in the future. Now if it's something like FEATURE_A_ENABLED but now that flag is being used for multiple things that don't really have anything to do with "feature A" then I understand.

> split the same damn check repeated 10x times into it’s own better named constant or helper function

Yes, abstracting out completely duplicated (no changes at all) code does make sense. The only pitfall here is if you have a "helper" function that has to take multiple arguments to handle special cases. The code should be identical, not "if you squint at it it looks similar". I've seen too many "abstractions" that are riddled with if/else if/else if/else if/else to handle the different things the calling code needs. At that point the minor duplication is much prefered. Similarly if I see a function that changes its return type based on a parameter I want to pull my hair out. Junior developer, myself included, fall prey to making things super DRY which actually can make the code more confusing and difficult to follow/reason about.

> as long as it doesn’t touch the business logic, who cares!

Here's the thing about that, even I still make mistakes (and will until I retire) when it comes to "this code is functionally the same". "doesn't touch the business logic" is a much harder thing to be sure of than one might think and unless you are intimately familiar with all the code calling through what you are changing can you really be sure? Literally just last week I made a mistake that got through multiple code reviews as "yep, that's functionally identical" but we missed a weird case further down that none of us had expected from reading the code. Also, see again: git history. I didn't always care as much about git history but I had a boss that instilled into me its value and now I fight to preserve it as it can be incredibly helpful in debugging or attempting to make a change to code (again, Chesterton's fence).

None of this is meant to be come across as attacking you are me telling you that I think you are wrong. Just how I see things, take from it what you will, ignore what you will. My way is not the perfect way, just the way I've learned over many years doing this professionally.

[0] https://en.wikipedia.org/wiki/G._K._Chesterton#Chesterton's_...

[pavel_lishin]

> That and touching 10+ things in their commits that were outside the scope of the ticket (“cleaning up other code”).

The smartest thing I do every day when I touch old code is turn off auto-formatting in my editor, studiously avoid doing minor clean-ups ("why the FUCK is this line 300 characters long"), and touch only what I'm working on.

Instead, I'll make a note to come back and clean up the other stuff later, if I have time, in a whole separate branch/commit/ticket. And then that note probably gets deleted, because this code is 8 years old, and is slated to be rewritten, almost certainly before I would get around to doing the minor chore clean-up.

It's not like the kitchen - there's no reason to quickly wipe up the stove during the 2 minutes that the microwave is doing its thing. It won't rot and stink up the place any more than it is now.

[joshstrange]

I couldn't agree more. Put in a ticket if you think there is a real problem that should be addressed but don't burn git history to fix indentation or something similarly minor. I have plenty of "rules" for my own code style but I don't touch code not within the scope of what I'm working on and I fit the style of the code I'm working in. I prefer camelCase but if the function has snake_case then I follow that style. And yes, turning off auto-formatting is a must in a legacy codebase that didn't have style guides/linters being used from the start.

It's way more important that I can see why a line was written, who wrote it, and when than spaces get turned into tabs.