3

I inherited 1 million lines of C++ legacy code. Throughout the code variables like bCPCHAR bPCHAR, bCPDOUBLE and bPINT are used. They are defined like this:

  • bCPCHAR: const char *
  • bPCHAR: char *
  • bCPDOUBLE: const double *
  • bPINT: int *

I was thinking of removing this level of indirection, especially because bCPCHAR and bPCHAR get mixed up in the code quite a lot, and are so much harder to read than const char * and char *. But a colleague pointed out that this change will generate lots of noise in git blame. Which I think is a valid point.

I think I'm not be the first person with this problem. Is there a solution to it? Can you give me some advice?

Clean Code from Uncle Bob didn't help me ;-)

  • 3
    Put up with the noise? I'm always a bit mystified why developers are so obsessed over warnings. If your boss had to choose between shipping and a quiet git blame, which one do you suppose he would choose? – Robert Harvey Dec 23 '17 at 15:54
  • 8
    Isn't it the same story with any cross cutting change that sweeps through the whole code base? There's a good reason to not want that to look like a typical change. – candied_orange Dec 23 '17 at 16:17
  • If I make the changes, the code gets simpler and git blame gets complicated. If I don't, the code stays complicated and git blame stays simple. @RobertHarvey: I can't see how that affects shipping. – Dominic Hofer Dec 23 '17 at 16:37
  • 4
    It affects shipping if it costs you time to solve the noise "problem." That is what your question asks for, isn't it? Personally, I agree with @CandiedOrange: this is a feature, not a problem. – Robert Harvey Dec 23 '17 at 16:39
  • 10
    Git, and any other version control system, is a tool to help you. Don't let a by product of a tool stop you from doing the right thing. Tools work for you. You do not work for the tool. – Caleb Dec 23 '17 at 17:33
10

I disagree that this is a counter-argument.

blame shows who last modified lines of code, not why or to what extent. That is a structural, unfixable weakness in blame, and everyone who uses it needs to be aware that it doesn't do what they might prefer (provide high-quality understanding of the history of a code base), no matter how much they wish it did. That kind of high-level narrative of the project must be conveyed through other channels in your project workflow. Certainly you should never change your coding practices to satisfy this fundamentally wrong and misguided assumption.

5

One option is to apply the changes you describe and live with a noisy git blame for a while, because, presumably, after some time it will become less noisy.

Another is to apply these changes over time, as a long-running process, e.g. as a preparatory step, limited in scope, before you start working on a new feature.

In any case, this might be of use: Git blame — prior commits?

  • +1 for noting that git allows you "blame" on previous versions. – Caleb Dec 23 '17 at 17:30
  • 1
    +1 for slowly deprecating the old defines as files are touched. There’s lots of risk to fixing this up as a sweeping change. – RubberDuck Dec 23 '17 at 20:33
  • 1
    @RubberDuck - great point about the risk of sweeping changes in something like this, especially if bPCHAR and bCPCHAR get mixed up as much as @PanicSheep suggests - it runs the risk of breaking code (especially workaround code, such as occasions where they've been swapped over, but only the problem has been detected and not traced to the swap itself). In any case, the old maxim of "test early, test often" holds true. – Myles Jan 3 '18 at 2:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.