refactor: simplify FormatSubVersion using strprintf/Join #30098

1. 
   theStack commented at 9:30 pm on May 13, 2024: contributor

   Rather than using std::ostringstream and manually joining the comments, use strprintf and our own Join helper.
2. 
   DrahtBot commented at 9:30 pm on May 13, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, hebasto, maflcko, tdb3
   Concept ACK	kevkevinpal

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

3. DrahtBot added the label Refactoring on May 13, 2024
4. refactor: simplify `FormatSubVersion` using strprintf/Join

   Rather than using std::ostringstream and manually joining the
   comments, use strprintf and our own `Join` helper.

   12d82817bf
5. theStack force-pushed on May 13, 2024
6. TheCharlatan approved
7. 
   TheCharlatan commented at 9:16 am on May 14, 2024: contributor

   tACK 12d82817bf32396b58c8c65645012def606680b6
8. in src/clientversion.cpp:68 in 12d82817bf

   76-        ss << ")";
   77-    }
   78-    ss << "/";
   79-    return ss.str();
   80+    std::string comments_str;
   81+    if (!comments.empty()) comments_str = strprintf("(%s)", Join(comments, "; "));
   

   ---

   ---

   

   hebasto commented at 9:42 am on May 14, 2024:

   nit: Placing a non-trivial if statement body on its own line can be useful for code coverage and debugging.

   ---

   

   maflcko commented at 11:11 am on May 14, 2024:

   why is the if needed at all? Can’t this whole function be a one-liner?

   0return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
   

   ---

   

   theStack commented at 11:20 am on May 14, 2024:

   why is the if needed at all? Can’t this whole function be a one-liner?

   0return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
   

   Using this expression would miss the parentheses around the comments; they should only be there if there are comments at all. The alternative is using a single return expression and the ternary operator, but I found this slightly less readable.

9. hebasto approved
10. 
    hebasto commented at 9:42 am on May 14, 2024: member

    ACK 12d82817bf32396b58c8c65645012def606680b6, I have reviewed the code and it looks OK.
11. 
    maflcko commented at 11:26 am on May 14, 2024: member

    utACK 12d82817bf32396b58c8c65645012def606680b6
12. tdb3 approved
13. 
    tdb3 commented at 1:13 am on May 15, 2024: contributor

    ACK for 12d82817bf32396b58c8c65645012def606680b6.

    Thank you. Making code more concise and readable is much appreciated. Built and ran unit tests (including util_tests/test_FormatSubVersion) and functionals. All passed.

    A one/same-line if aligns with the coding style (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c).

    If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces.

14. 
    kevkevinpal commented at 1:56 am on May 15, 2024: contributor

    utACK #30084.
15. fanquake merged this on May 15, 2024
16. fanquake closed this on May 15, 2024

    ---

17. theStack deleted the branch on May 15, 2024

Contributors
theStack DrahtBot TheCharlatan hebasto maflcko tdb3 kevkevinpal

Labels
Refactoring