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.

On Thursday, 30 December 2021 at 08:18:46 UTC+2, oguzism...@gmail.com wrote:
> 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.

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.

On Thursday, 30 December 2021 at 14:42:28 UTC+2, Öö Tiib wrote:
> On Thursday, 30 December 2021 at 08:18:46 UTC+2, oguzism...@gmail.com wrote:
> > 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.
> 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.

Nah scratch it. My bad i misread your code.

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"

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

=?UTF-8?B?T8SfdXo=?= <oguzismailuysal@gmail.com> writes:
>1. Define macros for `expr[expr_length - 1]', `expr[expr_length - 2]',
>etc. and use them instead. But, what am I gonna name them?

This is what you need to find out. For the rest of us
without knowledge of the actual meaning of those
expressions, it is even more difficult or impossible to find
good names. These names are the key to improved readability.

I would use functions. They are safer than macros, more
readable, and with inlining not slower either.

I'd start this way: In pseudocode, the code you posted has
the structure:

int f( void ){ return g()? h(): i(); }

with

inline int g( void )
{ return !expr[ expr_length - 1 ].is_number; }

,

inline int h( void )
{ if( skip_adj[ expr[ expr_length - 1 ].value][ op ])
return 0; }

, and

inline int i( void )
{ if
( expr_length > 2 &&
!expr[ expr_length - 2 ].is_number )
if( skip_alt[ expr[ expr_length - 2 ].value][ op ])return 0;
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 0;}

Now, we see that h and i do not always return a definite
value. This needs to be dealt with.

Then, the most important thing, replace "g", "h" and "i" by
good names!

Finally, apply this approach recursively to the function "i"
to decompose it into smaller functions as well.

On 12/30/2021 7:18 AM, Oğuz wrote:
> 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.

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)?

ram@zedat.fu-berlin.de (Stefan Ram) writes:
>These names are the key to improved readability.

Here's an example from a computer program I wrote in a
programming language called "Python". Before refactoring:

ad=600
mx = 1.2
lb = mx*ad
rb = lb + (mx-1)*ad
if i < lb:
cf = i/ad
elif i < rb:
wb = rb-lb
vb = (lb-rb)/mx
cf = (i-lb)/wb + (i-rb)/vb
elif n-i <( 10*ad if vib else ad ):
cf =( n - i )/( 10*ad if vib else ad )
else:
cf= 1

Before reading on, you can try to guess what it does!

After refactoring (some lines now have more than 72 characters):

(The "vib" part was removed since it was not needed anymore.)

def hull_curve_of( frame_number, integral_duration ):
'''
preconditions:
frame_number <= integral_duration
integral_duration > -1
/`-.
ADSR
attack-decay-sustain-release curve
attack: rise to maximum_intensity until end_of_attack,
decay: then fall to 1 until end_of_decay,
sustain: then, sustain the value of 1 until integral_duration - initial_duration,
release: finally, fall to 0 until integral_duration
'''
initial_duration = 600
maximum_intensity = 1.2
end_of_attack = maximum_intensity * initial_duration
end_of_decay = end_of_attack +( maximum_intensity - 1 )* initial_duration
if frame_number < end_of_attack: # A attack from 0 to maximum_intensity, reaching 1 at attack_duration
hull_curve = frame_number / initial_duration
elif frame_number < end_of_decay: # D decay from maximum_intensity to 1
width_of_decay = end_of_decay - end_of_attack
goes_from_0_to_1 =( frame_number - end_of_attack )/width_of_decay
goes_from_maximum_intensity_to_0 = maximum_intensity *( end_of_decay - frame_number )/ width_of_decay
hull_curve = goes_from_0_to_1 + goes_from_maximum_intensity_to_0
elif integral_duration - initial_duration < frame_number: # R release
hull_curve =( integral_duration - frame_number )/ initial_duration
else: # S sustain at 1
hull_curve = 1
return hull_curve

There is still room for improvement, but it's a start!

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;
}
}
}

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

Bonita Montero <Bonita.Montero@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.

On Thursday, December 30, 2021 at 6:18:46 AM UTC, oguzism...@gmail.com wrote:
> 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.

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;
}

ram@zedat.fu-berlin.de (Stefan Ram) writes:
>Now, we see that h and i do not always return a definite
>value. This needs to be dealt with.

I now see that my code does not the same as the original code.
In the case that none of the "return" statements would be
executed, the original code would simply continue to do what
follows it. I could not take that into consideration because
the code that follows it was not available to me.

So, my answer should not be taken literally (that's why
I wrote "pseudocode"). I just wanted to convey the general
idea of:

"Split it into meaningful functions with meaningful
names, and if that is not possible, reorganize the
code so that it becomes possible!".

Oğuz <oguzismailuysal@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.

ram@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(); }}

.

ram@zedat.fu-berlin.de (Stefan Ram) writes:
> ram@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.Thompson+u@gmail.com
Working, but not speaking, for Philips
void Void(void) { Void(); } /* The recursive call of the void */

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

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

Oğuz <oguzismailuysal@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)