Redundant behavior in coding guideline

[lovecreatesbeauty]

Some of the coding guideline are mandatory, and even the format or
layout of the text of the source code also should be followed. There's
plenty of codes like the following snippet.

Do you think the "re-evaluating the previous condition" is necessary
and should be avoided though it's one of your coding guideline? And
yes, there's plenty of code like this in many projects.



78 /* Error handling */
79 if ((NULL == p_SrcNm) || (NULL == p_DestNm))
80 {
81 lb_RetVal = false;
82
83 /* some other things special here ... */
84
85 if (NULL == p_SrcNm) /* Re-evaluating the previous
condition */
86 {
87 /* some other things special here ... */
88
89 ErrorEntryAdd(p_SrcNm);
90 ErrorHandle();
91 }
92
93 if (NULL == p_DestNm) /* Re-evaluating the previous
condition */
94 {
95 /* some other things special here ... */
96
97 ErrorEntryAdd(p_DestNm);
98 ErrorHandle();
99 }
100 }

 

[Skarmander]

Some of the coding guideline are mandatory, and even the format or
layout of the text of the source code also should be followed. There's
plenty of codes like the following snippet.

Do you think the "re-evaluating the previous condition" is necessary
and should be avoided though it's one of your coding guideline? And
yes, there's plenty of code like this in many projects.



78 /* Error handling */
79 if ((NULL == p_SrcNm) || (NULL == p_DestNm))
80 {
81 lb_RetVal = false;
82
83 /* some other things special here ... */
84
85 if (NULL == p_SrcNm) /* Re-evaluating the previous
condition */

No, it's not re-evaluating. (NULL == p_SrcNm) || (NULL == p_DestNm) does
not equal or even imply (NULL == p_SrcNm).

If you mean that the boolean expression (NULL == p_SrcNm) is
"recalculated", then yes. However, comparing a pointer to NULL is such a
trivial affair that it doesn't warrant a separate boolean to record the
outcome rather than just repeating the check.

86 {
87 /* some other things special here ... */
88
89 ErrorEntryAdd(p_SrcNm);
90 ErrorHandle();
91 }
92
93 if (NULL == p_DestNm) /* Re-evaluating the previous
condition */
94 {
95 /* some other things special here ... */
96
97 ErrorEntryAdd(p_DestNm);
98 ErrorHandle();
99 }
100 }

I'm pretty sure the calls to "ErrorEntryAdd" are wrong, because they're
just adding NULL pointers as whatever an "error entry" is. That seems
rather useless. Are you sure you didn't forget quotes? Don't you need to
pass a string with an error description or something?

S.

 

[Netocrat]

No, it's not re-evaluating. (NULL == p_SrcNm) || (NULL == p_DestNm) does
not equal or even imply (NULL == p_SrcNm).

If you mean that the boolean expression (NULL == p_SrcNm) is
"recalculated", then yes. However, comparing a pointer to NULL is such a
trivial affair that it doesn't warrant a separate boolean to record the
outcome rather than just repeating the check.

An alternative structuring would remove the outer conditional and repeat
the code indicated by the outermost "some other things special here"
(possibly including "lb_RetVal = false;"), within each inner conditional
body.

The drawbacks are dual-maintenance of that code and the likelihood of
extra generated machine code.

The dual-maintenance could be dealt with by encapsulating the code in a
function; the extra complexity is a drawback to this approach, and unless
the function were inlined, the function call overhead would likely exceed
that of the removed conditional.

Since this is error-handling code, performance is likely not critical and
the current structuring seems preferable.
[...]