refactor: remove redundant `for` constructs

yancyribbens commented at 5:05 pm on February 17, 2025: contributor

Remove redundant for loops. This refactor is a no-brainer.

DrahtBot commented at 5:05 pm on February 17, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31891.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Refactoring on Feb 17, 2025

jonatack commented at 5:35 pm on February 17, 2025: member

The test code is from 7488acc64685 in #27877 (cc @murchandamus). This change alters the order of adding clone coins, which doesn’t appear to matter, but maybe the author had a reason for this order.

yancyribbens commented at 5:51 pm on February 17, 2025: contributor

This change alters the order of adding clone coins, which doesn’t appear to matter, but maybe the author had a reason for this order.

The first things coin-grinder does is to sort the UTXO’s by effective value and weight, therefore, the order of the UTXO pool should not matter. Thanks for pointing this out and for taking a look.

refactor: remove redundant `for` constructs

Reduce code complexity by removing unneeded for loops.  Since the first
thing the tested algorithm coin-grinder does is to sort the inputs by
effective_value and weight, the order of the inputs in the test does not
matter.

9484a55a82

yancyribbens force-pushed on Feb 17, 2025

yancyribbens commented at 6:09 pm on February 17, 2025: contributor

I updated the commit message adding that the given input order does not matter since coin-grinder sorts first.

maflcko commented at 7:05 am on February 18, 2025: member

Tend toward NACK. This just makes the test harder to edit to adjust the counts for the amounts to be different. Either way, it is harmless and fine to keep as-is.

l0rinc commented at 11:55 am on February 18, 2025: contributor

Tend to agree with @maflcko, I vote to keep it as is, given that it’s already pushed

yancyribbens commented at 2:26 pm on February 18, 2025: contributor

Hmm ok. I thought this was a simple win since there’s no need to have 4 for loops here. Will keep it open for a bit longer to see if there is any other justification either way.

sipa commented at 2:29 pm on February 18, 2025: member

Performance doesn’t matter for tests.

maflcko commented at 2:29 pm on February 18, 2025: member

Will keep it open for a bit longer to see if there is any other justification either way.

The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.

maflcko closed this on Feb 18, 2025

maflcko commented at 2:30 pm on February 18, 2025: member

Thank you for your contribution. While this stylistic change can make sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

Time spent on review
Accidental introduction of bugs
(Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

If you wish to contribute in the future, please focus on creating high-quality, original content that demonstrates a clear understanding of the project’s requirements and goals. Also, see the contributing guidelines.

Leave a comment below, if you have any questions.

yancyribbens commented at 2:39 pm on February 18, 2025: contributor

The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.

The author was paged above and just thought he had justification or maybe it was just an oversight.

sipa commented at 2:47 pm on February 18, 2025: member

The author was paged

You are the author of this PR. You should have a justification for having it open.

yancyribbens commented at 2:58 pm on February 18, 2025: contributor

You are the author of this PR. You should have a justification for having it open.

The justification is that the for loops are redundant and confusing for contributors trying to understand the test. There is no good reason to do such a thing is my justification for opening this PR.

yancyribbens commented at 4:31 pm on February 18, 2025: contributor

@l0rinc I’m not sure why you’re thumbs downing my last comment. I understand your position that it’s already like this, so lets leave it and I appreciate the review. You don’t provide any reason for why this confusing code was added though. Personally I think code maintenance is a good thing, and the goal should be to help make it readable and understandable for humans.

l0rinc commented at 4:38 pm on February 18, 2025: contributor

@yancyribbens, please read the room and stop spamming. This change was very opinionated, changed one set of tradeoffs with some other ones (without any professional explanation) in a part of the code that isn’t read often enough for the review investment to be worth it. The changes and the reviews have opportunity costs that have to be weighed. The author of the PR should do 10x the work to simplify it to reviewers as much as possible, while This refactor is a no-brainer is condescending and obviates that you haven’t considered all angles (see comments) and haven’t done your necessary homework. Let’s not continue discussions here anymore, it’s not worth the effort (that’s why I only put a thumbs down without commenting).

yancyribbens commented at 10:06 pm on February 18, 2025: contributor

@l0rinc agreed, I’ll drop it. However, I appreciate the more in depth explanation since this helps me know for the future so I don’t make the mistakes. And yes, I agree that the comment This refactor is a no-brainer in retrospect was poor wording and I could have done better overall in a few ways. Anyway, thanks again for the detailed feedback.

refactor: remove redundant for constructs #31891

Code Coverage & Benchmarks

Reviews

refactor: remove redundant `for` constructs #31891