Clean code vs clean git/jira history

Question

I started reading the book Clean Code by Robert C. Martin and at the start I found this idea of his interesting, "Leave the code cleaner than you found it" adapted from the "Leave the campground cleaner than you found it". Now at my work in our code database I have this getter function in a class that gets a boolean value but is named getConnectionActive instead of isConnectionActive. I would like to rename it and leave the code a bit better.

A colleague that I asked about it pointed me to a rule of the company that conflicts. When we make git commits we are supposed to keep them as small as possible. This should make the commit easier understandable if somebody needs to read it and also, as far as possible keeps git blame pointing to the original author of some code. As an example they say that changing intendation is not good as it inflates the commit and changes git blame for all the lines. Intendation should be done right from the start.

So back to the method in question, if I change it in a commit that fixes another bug I would violate the companies rule as I would unnecessarily inflate the commit. However I cannot just make a commit of its own, as I always require a jira task number for a commit. So I would need to create a jira task only for changing this name. If done more often that would not only pollute the jira task history, it would still conflict with the companies rule of changing git blame, as it would no longer point to the commit that originally added this getConnectionActive for some reason.

This situation reminds me of this comic strip. How would you suggest handling this? Is it worth to try and change the companies rules? Or is it better to leave the method name as is? Or maybe even rewrite the original git commit in order to leave as little trace in the history as possible?

(I hope this belongs in this stackexchange. I was also thinking about the code review stackexchange but I don't really have code to review. I was also thinking about the workplace stackexchange but my companies rule doesn't seem so arbitrary that this problem is restricted to just my workplace.)

Answer 1

When we make git commits we are supposed to keep them as small as possible. This should make the commit easier understandable if somebody needs to read it and also, as far as possible keeps git blame pointing to the original author of some code

This policy includes one thing I kind of agree with, and one I disagree with:

---

You want to keep commits small. More precisely, you want to commit (and push, and merge) often so that everyone can stay up-to-date and merge issues are minimized, and you want your commits to be focused so that issues found in code review (or otherwise) won't prevent you from merging a desired part because of something unrelated.

You do not want to commit something that is too small to work - i.e. you don't want a "work in progress" commit that doesn't compile, or fails existing tests - at least not in the final history (see How to commit in-progress refactoring).

• When you make a small fix like get... -> is..., you want that change to propagate to others as soon as possible, so that no one ends up writing code that uses the old version. That change shouldn't have to wait for you to fix the rest of what you're working on. Furthermore, you probably want that change merged even if what you were otherwise working on turns out to be undesirable.

• Likewise, when your other work is finished just in time for some deadline, you don't want it to be rejected because, unbeknownst to you, the team has a policy of always using get for getters, regardless of the type.

Thus, I think it is a good idea to keep small fixes like this separated from the issue you are "actually" working on, and to get them merged ASAP. I would say that this kind of cleanup should be pushed pre-emptively even if your "real" work will touch the same file(s).

---

Keeping git blame pointing at the original author may sound attractive, but there are several problems:

• Between team discussions, pair programming, and code review, a single original author doesn't always exist.

• Given that people go on vacation, get sick, leave the company, get different responsibilities, or just forget things, you definitely don't want to encourage situations where the original author is the only one who interacts with a piece of code.

• For trivial lines like this, why would anyone be interested in the author?

• For non-trivial lines, the author of the last modification had better understand the code well enough, even if they weren't the original author.

• Knowing the original author doesn't really help you, if the code is readable and tested. Preserving knowledge about the author should never get in the way of these more important things!

• You want people to focus on providing the best value for the business. Squabbling over code ownership is not value, so any policy that elevates the original author is bad in that sense.

The original author can still be found by going back through history. For large-scale non-functional changes you might want to get funky with git to preserve the original author - maybe something like Automatic formatting (original by XXX), or perhaps just XXX.

---

So, in closing, you might need to talk to people about this first, but my preferred way forward would be:

• Make your fixes in a separate commit, and get it merged. If you need a Jira ID for this, create one. If you cannot, use the one for your current task and call it "cleanup to prepare for X"
• (Re-)Base your other work on the "fixed" commit

See Reconciling the Boy Scout Rule and Opportunistic Refactoring with code reviews for some tips on doing this in practice.

Answer 2

The real root cause of your problem is that two rules

• "I always require a jira task number for a commit", and

• "we are supposed to keep [commits] as small as possible"

are in conflict, since too fine-grained Jira tasks seem to be a problem for your team. This has nothing to do with refactoring and clean code, the latter just makes the conflict more visible.

However, the problem would also appear if you define Jira tasks which require several small commits, even if none of them contains a refactoring.

IMHO it would make more sense to associate your issue trackers task numbers with merges (where each merge can contain several small commits). I don't know if that is possible in your organization or with your current toolset, but I think you get the idea and may find a way to introduce it into your workflow.

Answer 3

Although the rules conflict in their literal form, they both have the same goal. Make the code easier to understand.

Just discuss changed with your colleagues before you make the change and i'm sure you wont go far wrong

Answer 4

Personally, I think the situation is fairly straight forward. All rules of thumb tend to be good until they meet internal policy. In other words, you do "Clean Coding" until the point where the Clean Code meets local policy. At which point local policy should always win out.

Mostly because there's a larger principle at play: Keeping code consistent. Consistency is more important than "keeping it clean". So if something is done a certain way, then you need to keep doing it that way so that people who develop in that system don't have to re-learn new processes over and over when the encapsulated functionality exists elsewhere but was changed because of the need to code cleanly.

As for Jira. Jira is used to track tasks, if there's a lot of tasks then there's a lot of Jiras. Avoiding maintenance tasks because there might be another Jira should not be a consideration. Jira helps us track our work, we should never limit our maintenance tasks because it create administrative overhead. The goal of a developer is manage and maintain code.

So for example

getConnectionActive instead of isConnectionActive

My question is this: Is the prefix "get" a part of policy? are there other functions that use this same style? Is there for example, a getSomeOtherFunction? Because if so, then do not change it, you're violating the underlying consistency of the code. In the future someone will see the two boolean returning functions and say "Ok, why is this a "get" and why is this an "is"? Is there some difference between these two pieces of code?

My general concern is, while yes we want to build as code as cleanly and as correctly as possible we need to also respect the consistency of the established system. Because inconsistencies create doubt and vagueness, which in turn means whoever is maintaining the code now has to investigate if the "is" and "get" functions are doing the same thing... or not... because one of the them is inconsistent with the rest of the code.