Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rant] Code duplication #317

Closed
klenze opened this issue Jan 29, 2020 · 3 comments
Closed

[rant] Code duplication #317

klenze opened this issue Jan 29, 2020 · 3 comments

Comments

@klenze
Copy link
Contributor

klenze commented Jan 29, 2020

For a fun exercise, run
find . -iname \*.cxx | xargs cat | grep -v include | gcc -E - | sed 's/^[ \t]*//' | sort | uniq -cd | sort -h | less

Pick a nontrivial line with a high duplication count, such as

gGeoManager->SetCurrentDirection(newdirection);

(which appears 36 times in the code), then run an appropriate grep pipe like in

grep -C 10 -F "gGeoManager->SetCurrentDirection(newdirection);" -r * | less

Notice how virtually the same block of code appears 36 times in our project. (kudos to xball/R3BXBall.cxx, for being the only ones to remove the //TGeoNode *bla comment line.)

I am not a software designer, but from my perspective, this does not seem like code which can be maintained easily. If you copy a file, chances are that both versions will slowly diverge, resulting in twice as much legacy code.

For a perhaps more recent example, take the fibers. At the moment, we have 17 directories matching "fi[0-9]+[ab]?". At the top level, no less. Each contains a different Mapped2Cal.h class. In my opinion, this raises the question: "what is unique about the calibration process of fiber detector 7?" I am not a fiber person, so I don't know. If fi7 channels are calibrated using a pol2, while all the other fibers use pol1, then that might be a good reason to have separate calibration code for fi7. If it is just "it uses different collection names from fi6", that is not a good reason, IMHO.

OOP (and C++ in particular with TMP) offers powerful tools for abstraction.
If handling all the fiber detectors in one task/collection is not an option, why not use

template <int N, char suffix='\0' >
class R3BFiber
{ ...
};

That way, one still has the power to specialize when needed (if fi7 needs a pol2 calibration or whatever) without creating 17x as much legacy code.

Backup copies in the repository are another pet peeve of mine. Take califa/unpack/oldCode. We have git. At the most, a comment in the new version "rewrite, see bf5282e for old version" should be enough.

TL;DR: commit code like you had to pay per line added.

Just my 2 cents. Sorry for the rant and apologies to the detectors I singled out.

@kresan
Copy link
Contributor

kresan commented Jan 30, 2020

Looking forward for not only creating "ranting" issues, but for a reasonable technical solution of code duplication problem and a corresponding PR.

@hapol
Copy link
Contributor

hapol commented Jan 30, 2020

Perfectly agree with the removal of backup copies. Please, proceed to remove califa/unpack/oldCode

@jose-luis-rs
Copy link
Contributor

I think that currently this problem is already solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants