Mu2e / Offline

Offline software for the Mu2e experiment
Apache License 2.0
8 stars 79 forks source link

APR 'findHelix' method indices are overloaded, needs comments and subdivision #1270

Open brownd1978 opened 1 month ago

brownd1978 commented 1 month ago

findHelix overloads integers used both as indices in a triple-nested loop and as flags for various conditions. The logical conditions should be split out as explicit flags. The loops should be refactored into functions, and the intent of this code should be described in comments.

gianipez commented 1 day ago

@matthewstortini was this solved?

matthewstortini commented 1 day ago

@matthewstortini was this solved?

@gianipez Not yet. I wanted to let Ethan finish the changes we are implementing first. Perhaps he can cancel his PR and I can address this problem in his area so that all of the changes can be in one PR.

I am not sure that the module needs to be refactored into functions as much as it may seem when one quickly scrolls through it. All of the logical steps are factorized now. One loop looks ugly mostly because there is a lot of debugging folded in. This debugging was for early development and uses MC info. It will all be removed in the short future.

kutschke commented 1 day ago

I would like to keep these as separate PRs so that they are reviewable.

Will this work: you fix the problem in this issue, make a PR that is reviewed and merged. Then Ethan can merge/rebase his PR and continue his work. If you do this, do you expect that Ethan will need to resolve extensive merge conflicts?

And to remind you. There are two ways to break out of a doubly nested loop. The strongly preferred way is to encapsulate the double loop in a function and just return from it. If there is a good reason to not do that, then use a goto.

kutschke commented 8 hours ago

@matthewstortini and @gianipez is there anything you would like clarified in my request?