refactor: reduce complexity of highly nested blocks #3633
Conversation
github-actions
bot
added
Component - Core
|
|
|
||
| return; |
There was a problem hiding this comment.
nitpick: void return at the end of the function
| } | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
Same comment as @andiwand gave above
| return true; | ||
| } | ||
|
|
||
| for (int j = -m_cfg.localMaxWindowSize; j <= m_cfg.localMaxWindowSize; j++) { |
There was a problem hiding this comment.
Is our test coverage high enough to be able to see if this is equivalent? /cc @goblirsc
| if (m_cfg.localMaxWindowSize == 0) { | ||
| return true; | ||
| } | ||
|
|
||
| for (int j = -m_cfg.localMaxWindowSize; j <= m_cfg.localMaxWindowSize; j++) { | ||
| if (y + j >= m_cfg.houghHistSize_y) { | ||
| continue; | ||
| } | ||
|
|
||
| for (int i = -m_cfg.localMaxWindowSize; i <= m_cfg.localMaxWindowSize; | ||
| i++) { | ||
| if (i == 0 && j == 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (x + i >= m_cfg.houghHistSize_x) { | ||
| continue; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).first < | ||
| houghHist.atLocalBins({y, x}).first) { | ||
| continue; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).first > | ||
| houghHist.atLocalBins({y, x}).first) { | ||
| return false; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).second.size() < | ||
| houghHist.atLocalBins({y, x}).second.size()) { | ||
| continue; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).second.size() > | ||
| houghHist.atLocalBins({y, x}).second.size()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (j <= 0 && i <= 0) { | ||
| return false; // favor bottom-left (low phi, low neg q/pt) | ||
| } |
There was a problem hiding this comment.
This is a case where I am really not sure if the right hand side is actually more readable than the left hand side. The mental load of tracking early returns and continues is quite significant.
| if (!accept) { | ||
| break; | ||
| } | ||
| for (auto y : std::vector<double>{-dy, dy}) { |
There was a problem hiding this comment.
This is so you abort earlier. However, this is not at all performance critical code, so readability might be preferable over an earlier abort.
There was a problem hiding this comment.
since the loops seem independent something like a product iteration like in python might be nice. then the control flow is easier to handle with a single break. otherwise I agree with paul that the additional looping is not in a critical path so the old version might be preferable
| if (axisTypeA == AxisType::Equidistant && | ||
| axisTypeB == AxisType::Equidistant) { | ||
| auto [rangeA, binsA] = eqExtractor(jAxisA); | ||
| auto [rangeB, binsB] = eqExtractor(jAxisB); | ||
| EqBoundEqClosed ebecAG{rangeA, binsA, rangeB, binsB}; | ||
| auto grid = GridJsonConverter::fromJson<EqBoundEqClosed, ValueType>( | ||
| jGrid, ebecAG); | ||
|
|
||
| return generator.createUpdater(std::move(grid), {bValueA, bValueB}, | ||
| transform); | ||
| } | ||
|
|
||
| if (axisTypeA == AxisType::Equidistant) { | ||
| auto [rangeA, binsA] = eqExtractor(jAxisA); | ||
| EqBoundVarClosed ebvcAG{rangeA, binsA, vExtractor(jAxisB)}; | ||
| auto grid = GridJsonConverter::fromJson<EqBoundVarClosed, ValueType>( | ||
| jGrid, ebvcAG); | ||
|
|
||
| return generator.createUpdater(std::move(grid), {bValueA, bValueB}, | ||
| transform); | ||
| } | ||
|
|
||
| if (axisTypeB == AxisType::Equidistant) { | ||
| auto [rangeB, binsB] = eqExtractor(jAxisB); | ||
| VarBoundEqClosed vbecAG{vExtractor(jAxisA), rangeB, binsB}; | ||
| auto grid = GridJsonConverter::fromJson<VarBoundEqClosed, ValueType>( | ||
| jGrid, vbecAG); | ||
|
|
||
| return generator.createUpdater(std::move(grid), {bValueA, bValueB}, | ||
| transform); | ||
| } |
There was a problem hiding this comment.
This can be an else-if construction.
| if (!accept) { | ||
| break; | ||
| } | ||
| for (auto y : std::vector<double>{-dy, dy}) { | ||
| if (!accept) { | ||
| break; | ||
| } | ||
| for (auto z : std::vector<double>{-dz, dz}) { | ||
| Vector3 edge = etrf * Vector3(x, y, z); | ||
| for (auto& check : options.parseRanges) { | ||
| double val = VectorHelpers::cast(edge, check.first); | ||
| if (val < check.second.first || val > check.second.second) { | ||
| if (!accept) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Can't these breaks be put at the end of the inner loop body?
|
|
||
| if (a.direction() == direction) { | ||
| std::size_t nBinsA = locBinsA.at(0); | ||
| std::size_t nBinsB = locBinsB.at(0); | ||
| std::size_t nBinsCommon = locBinsB.at(1); | ||
|
|
||
| for (std::size_t i = 1; i <= nBinsA; ++i) { | ||
| for (std::size_t j = 1; j <= nBinsCommon; ++j) { | ||
| merged.atLocalBins({i, j}) = a.atLocalBins({i, j}); | ||
| } | ||
| } | ||
|
|
||
| for (std::size_t i = 1; i <= nBinsCommon; ++i) { | ||
| for (std::size_t j = 1; j <= nBinsB; ++j) { | ||
| std::size_t tj = j + nBinsA; | ||
| merged.atLocalBins({i, tj}) = b.atLocalBins({i, j}); | ||
| } | ||
| for (std::size_t i = 1; i <= nBinsB; ++i) { | ||
| for (std::size_t j = 1; j <= nBinsCommon; ++j) { | ||
| std::size_t ti = i + nBinsA; | ||
| merged.atLocalBins({ti, j}) = b.atLocalBins({i, j}); | ||
| } | ||
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
This could be an else-if construct, avoiding an early return.
|
Regarding this use of |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
No description provided.