<p><a class="user-mention" data-hovercard-type="user" data-hovercard-url="/users/sqrrm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/sqrrm">@sqrrm</a> wrote:</p>
<blockquote>
<p>[it] would still be good to address the braces.</p>
</blockquote>
<p>First some data:</p>
<p>Of 171 <code>for</code> loops in the codebase, 12 (7%) are written without braces:</p>
<pre><code>~/dev/bisq[master]
$ git grep 'for (.*) {$' | wc -l
171
~/dev/bisq[master]
$ git grep 'for (.*)$' | wc -l
15
</code></pre>
<p>Note that 3 of the 15 above are actually aberrations in which the brace is on a newline (<a class="user-mention" data-hovercard-type="user" data-hovercard-url="/users/wiz/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/wiz">@wiz</a>, I'm looking at you), so the total number is actually 12.</p>
<p>Of 3346 <code>if</code> statements in the codebase, 1796 (53%) are brace-free.</p>
<pre><code>~/dev/bisq[master]
$ git grep 'if (.*) {$' | wc -l
3346
~/dev/bisq[master]
$ git grep 'if (.*)$' | wc -l
1796
</code></pre>
<p>As far as I am aware, we have not written down any rules on this. There is nothing in the bisq-network/style repository's issues, for example.</p>
<p>In the absence of any explicit rule, I would generally agree that the one should fall back to the default rule to "follow convention" and thus the <code>for</code> loops should have braces added, and the <code>if</code> statements may be left as-is given that more than half of all such statements in the codebase already take this approach.</p>
<p>With that said, I'd like to argue for embracing braceless <code>for</code> loops under certain conditions on the grounds of elegance, aesthetics, readability and concision. Before we get there, though, consider the strong case for using braceless <code>if</code> statements in guard logic that you want to be as concise and readable as possible, e.g.:</p>
<div class="highlight highlight-source-java"><pre><span class="pl-k">public</span> <span class="pl-k">void</span> myMethod(<span class="pl-smi">String</span> arg) {
<span class="pl-k">if</span> (arg <span class="pl-k">==</span> <span class="pl-c1">null</span>)
<span class="pl-k">throw</span> <span class="pl-k">new</span> <span class="pl-smi">IllegalArgumentException</span>(<span class="pl-c1">...</span>);
<span class="pl-c"><span class="pl-c">//</span> normal logic follows</span>
}</pre></div>
<p>To add braces around the guard logic is simply to add line noise. One wants the eye to flow over the guard logic, understanding it intuitively as guard logic, and to then get to the meat of the actual method. Adding braces makes it feel "heavier", subtly making the reader wonder whether there might be something there that requires more thought than: "this is just a null check".</p>
<p>I would argue the same for the <code>for</code> loops in question in this PR. While they do not represent guard logic, they have a clear and focused purpose, and the ability to write the loop without braces expresses the elemental nature of the logic within; that there is no fat, nothing extra, no side effects, just pure iteration, filtering and returning. For example here is such a <code>for</code> loop from <code>CompositeOptionSet</code> in this PR:</p>
<div class="highlight highlight-source-java"><pre><span class="pl-k">public</span> <span class="pl-k">boolean</span> has(<span class="pl-k">OptionSpec<?></span> option) {
<span class="pl-k">for</span> (<span class="pl-smi">OptionSet</span> optionSet <span class="pl-k">:</span> optionSets)
<span class="pl-k">if</span> (optionSet<span class="pl-k">.</span>has(option))
<span class="pl-k">return</span> <span class="pl-c1">true</span>;
<span class="pl-k">return</span> <span class="pl-c1">false</span>;
}</pre></div>
<p>I argue that nothing beneficial comes from adding braces here, and that doing so is to follow a convention blindly, purely for the sake of consistency, and at the cost of concision, readability and clarity.</p>
<p>Now let's consider a counterexample. The following is also taken from this PR in <a href="https://github.com/bisq-network/bisq/pull/3889/files/b34d59c0a930e436b0b542114c48005ff857d4c8#diff-815a90a6c6837bde43ee9af6d026e336R36-R50"><code>ConfigFileEditor#clearOption</code>, lines 36-50</a>:</p>
<div class="highlight highlight-source-java"><pre><span class="pl-k">for</span> (<span class="pl-smi">String</span> line <span class="pl-k">:</span> lines) {
<span class="pl-k">if</span> (<span class="pl-smi">ConfigFileOption</span><span class="pl-k">.</span>isOption(line)) {
<span class="pl-smi">ConfigFileOption</span> existingOption <span class="pl-k">=</span> <span class="pl-smi">ConfigFileOption</span><span class="pl-k">.</span>parse(line);
<span class="pl-k">if</span> (existingOption<span class="pl-k">.</span>name<span class="pl-k">.</span>equals(name)) {
fileAlreadyContainsTargetOption <span class="pl-k">=</span> <span class="pl-c1">true</span>;
<span class="pl-k">if</span> (<span class="pl-k">!</span>existingOption<span class="pl-k">.</span>arg<span class="pl-k">.</span>equals(arg)) {
<span class="pl-smi">ConfigFileOption</span> newOption <span class="pl-k">=</span> <span class="pl-k">new</span> <span class="pl-smi">ConfigFileOption</span>(name, arg);
writer<span class="pl-k">.</span>println(newOption);
log<span class="pl-k">.</span>warn(<span class="pl-s"><span class="pl-pds">"</span>Overwrote existing config file option '{}' as '{}'<span class="pl-pds">"</span></span>, existingOption, newOption);
<span class="pl-k">continue</span>;
}
}
}
writer<span class="pl-k">.</span>println(line);
}
<span class="pl-c"><span class="pl-c">//</span> additional logic follows</span></pre></div>
<p>It so happens that the way this is written, it would cause logic and/or compiler errors if any of these braces were left out, but consider if it had been written slightly differently and the author chose to eliminate braces wherever possible (note that this code is wrong and not functionally equivalent to the above; the changes I'm making are just for the purposes of illustration):</p>
<div class="highlight highlight-source-java"><pre><span class="pl-k">for</span> (<span class="pl-smi">String</span> line <span class="pl-k">:</span> lines)
<span class="pl-k">if</span> (<span class="pl-smi">ConfigFileOption</span><span class="pl-k">.</span>isOption(line)) {
<span class="pl-smi">ConfigFileOption</span> existingOption <span class="pl-k">=</span> <span class="pl-smi">ConfigFileOption</span><span class="pl-k">.</span>parse(line);
<span class="pl-k">if</span> (existingOption<span class="pl-k">.</span>name<span class="pl-k">.</span>equals(name)) {
fileAlreadyContainsTargetOption <span class="pl-k">=</span> <span class="pl-c1">true</span>;
<span class="pl-k">if</span> (<span class="pl-k">!</span>existingOption<span class="pl-k">.</span>arg<span class="pl-k">.</span>equals(arg)) {
<span class="pl-smi">ConfigFileOption</span> newOption <span class="pl-k">=</span> <span class="pl-k">new</span> <span class="pl-smi">ConfigFileOption</span>(name, arg);
writer<span class="pl-k">.</span>println(newOption);
log<span class="pl-k">.</span>warn(<span class="pl-s"><span class="pl-pds">"</span>Overwrote existing config file option '{}' as '{}'<span class="pl-pds">"</span></span>, existingOption, newOption);
}
<span class="pl-k">else</span>
writer<span class="pl-k">.</span>println(line);
}
}
<span class="pl-c"><span class="pl-c">//</span> additional logic follows</span></pre></div>
<p>In the latter example above, the author has omitted braces from the <code>for</code> loop and from the <code>else</code> statement, while all other required braces remain in place. <strong>This is the kind of madness we are trying to avoid when we make rules about using braces.</strong> In a complex block of logic like the above, selectively omitting braces makes the whole construction much harder to read and reason about. Frankly, I wouldn't want anyone who thinks this is a good idea to be committing to the Bisq codebase. Nor do I want to prevent any such possibility of this by decreeing an (IMO) dumbed-down style rule like "always use braces". The real rule here is something more like "use braceless loops and conditionals judiciously", where your judgement demonstrates you know the difference between the good and bad examples above. If you don't it's a red flag about your coding skill and no overly strict set of style rules is going to save you.</p>
<p>So: lest this become a bikeshed session, I propose we leave the braceless <code>for</code> loops as-in in this PR on the merit of the arguments above, and that if strong opinions otherwise present themselves, that we take it to Keybase, a bisq-network/style discussion or similar. In any case, I am happy to write a style rule (shorter than the text above) titled something like "Use braceless loops and conditionals judiciously" in order to make our guidelines about this clear. If the conversation continues and we end up at a rough consensus that says we should in fact always use braces in <code>for</code> loops, I'll come back and revise this code accordingly. I'd appreciate not holding up the merge on it in the meantime, though.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/3889?email_source=notifications&email_token=AJFFTNQXDWZXRO7EQKDMG2TQ64LOPA5CNFSM4KFNLJBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQHYUQ#issuecomment-576748626">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNSZPEIMK2FYJVIEKXTQ64LOPANCNFSM4KFNLJBA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNR7ZUV2BH3KZBJVDYTQ64LOPA5CNFSM4KFNLJBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQHYUQ.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/3889?email_source=notifications\u0026email_token=AJFFTNQXDWZXRO7EQKDMG2TQ64LOPA5CNFSM4KFNLJBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQHYUQ#issuecomment-576748626",
"url": "https://github.com/bisq-network/bisq/pull/3889?email_source=notifications\u0026email_token=AJFFTNQXDWZXRO7EQKDMG2TQ64LOPA5CNFSM4KFNLJBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQHYUQ#issuecomment-576748626",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>