L A Walsh wrote: 1) I see no benefit in the use of extra braces. It diminishes > comprehension in this case.
really? to me the braces make the code easier to read, in the context of the surrounding function. they clarify the intention. * if (THE VALUE IS OK) {* * USE THE VALUE* * }* * else {* * EMIT AN ERROR MESSAGE* * }* the underlying logic is the same as the pre-existing code. all i changed was the *EMIT AN ERROR MESSAGE* part. because it then became an if-statement in its own right, i added the surrounding braces. in many coding standards, the braces would be required. although i didn't raise this as an issue, i don't think it's an improvement either. the alterations that i question are: * changing the variable name from check_nullness (as in the calling routine) to check_null. * flipping the if from positive to negative * changing the condition from a boolean test to a comparison. * and, outside of source code itself, dropping the tests. Owners that require submitters to > always use the owner's exact formatting (including tab/space > and indentation preferences) will tend to dissuade contributions. as a first-time submitter, i'm feeling somewhat discouraged by alterations that i think lower the quality of the code, and especially by the dropped tests. i don't care so much about the formatting. On Tue, Mar 6, 2018 at 9:51 AM, L A Walsh <b...@tlinx.org> wrote: > > > don fong wrote: > >> my patch (form (A)): >> >> - report_error (_("%s: parameter null or not set"), name); >> + { >> + if (check_nullness) >> + report_error (_("%s: parameter null or not set"), name); >> + else >> + report_error (_("%s: parameter is not set"), name); >> + } >> >> the new code (form (B)): >> >> else if (check_null == 0) >> report_error (_("%s: parameter not set"), name); >> else >> report_error (_("%s: parameter null or not set"), name); >> >> > 1) I see no benefit in the use of extra braces. It diminishes > comprehension in this case. > > 2) The purpose appears to, optionally treat a null value for ptr name > as either an "unset" condition (as in never set) vs. mentioning > that it might also be the case that it might have been set, but with > a null value. > > OTOH, if the purpose is to vary the error message, I might find > 1 call to be more clear: > > report_error( check_null ? _("%s: parameter null or not set") > : _("%s: parameter not set"), name ); > > Whether or not 'name )' would be on a separate line, or set off > with extra spaces would depend on code width compared with surrounding > lines. That also assumes whether or not the macro '_()' can be > used more than once within the call to report_error. It might be > that the format above would exceed some desired code width, which > might "recommend" a different formatting. > > It's hard to say w/o knowing other conventions in the code. > > However, personally, I'd find the fact that it was accepted and > simply adapted to whatever the code owner wanted in this situation, an > overriding benefit. Owners that require submitters to > always use the owner's exact formatting (including tab/space > and indentation preferences) will tend to dissuade contributions. > > I have things that are more clear for me to read and comprehend, > while others have their own "input format" that benefit them as well. > Ideally, computers can be used to automatically reformat such > differences as code is imported or exported. > -l > > > > >