79 views

Skip to first unread message

Dec 30, 2021, 1:18:46 AM12/30/21

to

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.

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.

Dec 30, 2021, 7:42:28 AM12/30/21

to

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.

Dec 30, 2021, 7:47:29 AM12/30/21

to

Dec 30, 2021, 10:43:46 AM12/30/21

to

On 30/12/2021 12:47, Öö Tiib wrote:

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

That's what he said!.
> Nah scratch it. My bad i misread your code.

"barely readable even with comments"

Dec 30, 2021, 11:24:54 AM12/30/21

to

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
>

Dec 30, 2021, 12:34:32 PM12/30/21

to

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

Dec 30, 2021, 1:14:18 PM12/30/21

to

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

> }

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

}

}

}

Dec 30, 2021, 1:44:00 PM12/30/21

to

Your coding-style is ugly as hell.

That would be a disqualifcation for any job.

That would be a disqualifcation for any job.

Dec 30, 2021, 2:59:15 PM12/30/21

to

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.
> Your coding-style is ugly as hell.

> That would be a disqualifcation for any job.

The USENET Guide, chapter 2, section 2.71828182845904523536, ``how to

make new friends on the USENET'', Mark V. Shaney, 1993.

Dec 30, 2021, 3:15:41 PM12/30/21

to

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;
&& expr[expr_length - 3].value < expr[expr_length - 1].value

}

Dec 30, 2021, 5:01:21 PM12/30/21

to

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. [...]
> 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

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

Dec 30, 2021, 7:22:52 PM12/30/21

to

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 */

> 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 */

Dec 31, 2021, 1:06:02 AM12/31/21

to

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

> }

>

>

Dec 31, 2021, 1:11:42 AM12/31/21

to

On 12/31/21 1:01 AM, Tim Rentsch wrote:

> [...] My sense

yeah, I have to admit that your example looks better. Thanks

> [...] 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
> white space, both horizontal and vertical, is a key element of

> making the code easier to take in and comprehend.

>

yeah, I have to admit that your example looks better. Thanks

Jan 2, 2022, 8:28:16 AMJan 2

to

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)

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)

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu