A question about style

[Oğuz]

I have this if-else thingy:

if (!expr[expr_length - 1].is_number) {
if (skip_adj[expr[expr_length - 1].value][op])
return false;
}
else if (expr_length > 2 && !expr[expr_length - 2].is_number) {
if (skip_alt[expr[expr_length - 2].value][op])
return false;
else if (expr[expr_length - 3].is_number
&& expr[expr_length - 3].value < expr[expr_length - 1].value
&& skip_alt_asc[expr[expr_length - 2].value][op])
return false;
}

(`expr' is a fixed-size array of structs, I increase `expr_length' as I
fill it in. `skip_*' are two dimensional arrays of booleans, but I don't
have any problem regarding them.)

And it looks ugly; barely readable even with comments. I want it to be
readable at least. Possible emendations I can think of are:

1. Define macros for `expr[expr_length - 1]', `expr[expr_length - 2]',
etc. and use them instead. But, what am I gonna name them?

2. Define a macro `expr_end' for `(expr + expr_length)', and use
`expr_end[-1]', `expr_end[-2]', etc. instead. But I'm afraid this might
still be hard to understand for someone who's not familiar with the code
(e.g. me 3 months later from now).

So, do you guys have better ideas for making this piece of crap a bit
more readable (other than rewriting it in C++)? Thanks in advance.

[Öö Tiib]

Ugly or not ugly is matter of taste but you seem to try to bike-shed
into aesthetics instead of fixing logical issues?

See: One condition of yours detects that expr_length > 2 its else
however (so case when expr_length is 0, 1 or 2?) does check
expr[expr_length - 3].is_number. That means expr[-3].is_number,
expr[-2].is_number or expr[-1].is_number. So either it was
known before that expr_length > 2 always and so the check was
pointless or you have undefined behavior.

[Öö Tiib]

Nah scratch it. My bad i misread your code.

[James rock]

On 30/12/2021 12:47, Öö Tiib wrote:
> Nah scratch it. My bad i misread your code.

That's what he said!.

"barely readable even with comments"

[Oğuz]

On 12/30/21 3:47 PM, Öö Tiib wrote:
> [...]

> Nah scratch it. My bad i misread your code.
>

No problem. At least now I know it's worse than I thought

[Manfred]

Maybe Öö Tiib still has a point: it's probably more about the logic than
aesthetics - I mean, your code might be logically correct, but it is the
underlying model that is not clear.
Often, in this kind of thing, a clear code is the result of:
- clear requirements
- clear logic modeled upon such requirements
- then, translation of model into code is often also clear.

What makes me wonder most about those lines is that all that if/else
intricacy is done to yield a true/false flag - is this really the best
possible design of what you are trying to do (whatever it may be)?

[Bart]

On 30/12/2021 06:18, Oğuz wrote:
> if (!expr[expr_length - 1].is_number) {
>   if (skip_adj[expr[expr_length - 1].value][op])
>     return false;
> }
> else if (expr_length > 2 && !expr[expr_length - 2].is_number) {
>   if (skip_alt[expr[expr_length - 2].value][op])
>     return false;
>   else if (expr[expr_length - 3].is_number
>       && expr[expr_length - 3].value < expr[expr_length - 1].value
>       && skip_alt_asc[expr[expr_length - 2].value][op])
>     return false;
> }

Assuming expr is some sort of stack:

#define A (expr[expr_length-1])
#define B (expr[expr_length-2])
#define C (expr[expr_length-3])

int dummy(void) {
if (!A.is_number) {
if (skip_adj[A.value][op])
return false;
}
else if (expr_length > 2 && !B.is_number) {
if (skip_alt[B.value][op]) {
return false;
}
else if (C.is_number
&& C.value < A.value
&& skip_alt_asc[B.value][op]) {
return false;
}
}
}

[Bonita Montero]

Your coding-style is ugly as hell.
That would be a disqualifcation for any job.

[Meredith Montgomery]

Bonita Montero <Bonita....@gmail.com> writes:

> Your coding-style is ugly as hell.
> That would be a disqualifcation for any job.

Lol. You forget to cite the source of this quote. Let me help you.

The USENET Guide, chapter 2, section 2.71828182845904523536, ``how to
make new friends on the USENET'', Mark V. Shaney, 1993.

[Paul N]

How about something like the following? I imagine some of the variables are locals rather than globals so would need to be passed as arguments but this is a rough idea.

int skip(int type, int val, int op) {
switch(type) {
case 1: return skip_adj[val][op];
case 2: return skip_alt[val][op];
default: return skip_alt_asc[val][op]; }

int is_number(int len, int pos) {
return len > pos && !expr[len - pos].is_number; }

...
if (is_number(expr_length, 1)) { // this will do a check on length that the original didn't
if (skip(1, expr[expr_length - 1].value, op)) return false; }
else if (is_number(expr_length, 2) {
if (skip(2, expr[expr_length - 2].value, op)) return false;

else if (expr[expr_length - 3].is_number
&& expr[expr_length - 3].value < expr[expr_length - 1].value

&& skip(3, expr[expr_length - 2].value, op) return false;
}

[Tim Rentsch]

Oğuz <oguzism...@gmail.com> writes:

> I have this if-else thingy:
>
> if (!expr[expr_length - 1].is_number) {
> if (skip_adj[expr[expr_length - 1].value][op])
> return false;
> }
> else if (expr_length > 2 && !expr[expr_length - 2].is_number) {
> if (skip_alt[expr[expr_length - 2].value][op])
> return false;
> else if (expr[expr_length - 3].is_number
> && expr[expr_length - 3].value < expr[expr_length - 1].value
> && skip_alt_asc[expr[expr_length - 2].value][op])
> return false;
> }
>
> (`expr' is a fixed-size array of structs, I increase `expr_length'
> as I fill it in. `skip_*' are two dimensional arrays of booleans,
> but I don't have any problem regarding them.)
>
> And it looks ugly; barely readable even with comments. I want it

> to be readable at least. [...]

Putting the code in the context of a function body (with a
'return 5;' at the end just to pick a value obviously not
the same as 'false'):

int
original_code(){

if (!expr[expr_length - 1].is_number) {
if (skip_adj[expr[expr_length - 1].value][op])
return false;
}
else if (expr_length > 2 && !expr[expr_length - 2].is_number) {
if (skip_alt[expr[expr_length - 2].value][op])
return false;
else if (expr[expr_length - 3].is_number
&& expr[expr_length - 3].value < expr[expr_length - 1].value
&& skip_alt_asc[expr[expr_length - 2].value][op])
return false;
}

return 5;
}

Assuming no major changes to the code (so stipulating any changes
be kept local to this one function), and taking 'expr' to be an
array of type 'Expression', I would try something along these
lines:

int
possible_revision(){
Expression *e = expr + expr_length;

if( e[-1].is_number ){
if( skip_adj[ e[-1].value ][op] ) return false;

} else if( expr_length > 2 && e[-2].is_number ){
if(
skip_alt[ e[-2].value ][op] || (
e[-3].is_number &&
e[-3].value < e[-1].value &&
skip_alt_asc[ e[-2].value ][op]
)
){
return false;
}
}

return 5;
}

To me this way of writing is cleaner and easier to take in. I
have the sense that I can follow what is being done, even if
exactly what is being accomplished for the caller isn't evident.
Choosing a good name for the function might to a long way towards
addressing the latter question.

Different people have different reactions to various lexical
styles, so that aspect may be somewhat off putting here. If so
then perhaps the example will spark some other ideas. My sense
for this question is that a judicious choice of where to insert
white space, both horizontal and vertical, is a key element of
making the code easier to take in and comprehend.

[Keith Thompson]

r...@zedat.fu-berlin.de (Stefan Ram) writes:
> r...@zedat.fu-berlin.de (Stefan Ram) writes:
>>"Split it into meaningful functions with meaningful
>>names, and if that is not possible, reorganize the
>>code so that it becomes possible!".
>
> So, for example:
>
> int f( void )
> {
> if( rand() < 1 )
> { if( rand() < 2 )return 0;
> /* else fall through to continuation */ }
> else
> { if( rand() < 3 )return 0;
> /* else fall through to continuation */ }
>
> /* continuation */
> puts( "A" );
> puts( "B" );
> puts( "C" );
> return 1; }
>
> would become:
>
> int f( void )
> { if( rand() < 1 )
> { if( rand() < 2 )return 0;
> else
> { puts( "A" );
> puts( "B" );
> puts( "C" );
> return 1; }}
> else
> { if( rand() < 3 )return 0;
> else
> { puts( "A" );
> puts( "B" );
> puts( "C" );
> return 1; }}}
>
> . Now, we have code duplication, but then the next step is:
>
> inline int continuation( void )
> { puts( "A" );
> puts( "B" );
> puts( "C" );
> return 1; }
>
> int f( void )
> { if( rand() < 1 )
> { return rand() < 2? 0: continuation();
> else
> { return rand() < 3? 0: continuation(); }}

You have an unmatched curly brace on that last function definition.

Your unconventional layout makes that very difficult to see.

--
Keith Thompson (The_Other_Keith) Keith.S.T...@gmail.com
Working, but not speaking, for Philips
void Void(void) { Void(); } /* The recursive call of the void */

[Oğuz]

On 12/30/21 11:15 PM, Paul N wrote:
> How about something like the following? I imagine some of the variables are locals rather than globals so would need to be passed as arguments but this is a rough idea.
>
> int skip(int type, int val, int op) {
> switch(type) {
> case 1: return skip_adj[val][op];
> case 2: return skip_alt[val][op];
> default: return skip_alt_asc[val][op]; }
>
> int is_number(int len, int pos) {
> return len > pos && !expr[len - pos].is_number; }
>
> ...
> if (is_number(expr_length, 1)) { // this will do a check on length that the original didn't
> if (skip(1, expr[expr_length - 1].value, op)) return false; }
> else if (is_number(expr_length, 2) {
> if (skip(2, expr[expr_length - 2].value, op)) return false;
> else if (expr[expr_length - 3].is_number
> && expr[expr_length - 3].value < expr[expr_length - 1].value
> && skip(3, expr[expr_length - 2].value, op) return false;
> }
>
>

Yea, I think I'll do it this way. Thank you

[Oğuz]

On 12/31/21 1:01 AM, Tim Rentsch wrote:
> [...] My sense

> for this question is that a judicious choice of where to insert
> white space, both horizontal and vertical, is a key element of
> making the code easier to take in and comprehend.
>

I'm not a fan of horizontal spaces (as in `if( cond )' etc.), but
yeah, I have to admit that your example looks better. Thanks

[Jens Schweikhardt]

Oğuz <oguzism...@gmail.com> wrote
in <sqjivq$get$1...@oguzismail.eternal-september.org>:
...
# 1. Define macros for `expr[expr_length - 1]', `expr[expr_length - 2]',
# etc. and use them instead. But, what am I gonna name them?

You are trying to solve one of the two unsolved problems in computer science.
1. Cache coherency
2. Naming identifiers
3. Off-by-one errors

:-)

expr_last and expr_almost_last?

Regards,

Jens
--
Jens Schweikhardt http://www.schweikhardt.net/
SIGSIG -- signature too long (core dumped)