Re: coding standards

[don fong]

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 <bash@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
>
>
>
>
>