Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c > #164114 > unrolled thread

A question about style

Back to article view | Back to comp.lang.c

Contents

  A question about style Oğuz <oguzismailuysal@gmail.com> - 2021-12-30 09:18 +0300
    Re: A question about style Öö Tiib <ootiib@hot.ee> - 2021-12-30 04:42 -0800
      Re: A question about style Öö Tiib <ootiib@hot.ee> - 2021-12-30 04:47 -0800
        Re: A question about style James rock <jamesroack12@gmail.com> - 2021-12-30 15:40 +0000
        Re: A question about style Oğuz <oguzismailuysal@gmail.com> - 2021-12-30 19:24 +0300
    Re: A question about style Manfred <noname@add.invalid> - 2021-12-30 18:34 +0100
    Re: A question about style Bart <bc@freeuk.com> - 2021-12-30 18:14 +0000
    Re: A question about style Bonita Montero <Bonita.Montero@gmail.com> - 2021-12-30 19:43 +0100
      Re: A question about style Meredith Montgomery <mmontgomery@levado.to> - 2021-12-30 16:56 -0300
    Re: A question about style Paul N <gw7rib@aol.com> - 2021-12-30 12:15 -0800
      Re: A question about style Oğuz <oguzismailuysal@gmail.com> - 2021-12-31 09:05 +0300
    Re: A question about style Tim Rentsch <tr.17687@z991.linuxsc.com> - 2021-12-30 14:01 -0800
      Re: A question about style Oğuz <oguzismailuysal@gmail.com> - 2021-12-31 09:11 +0300
    Re: A question about style Keith Thompson <Keith.S.Thompson+u@gmail.com> - 2021-12-30 16:22 -0800
    Re: A question about style Jens Schweikhardt <usenet@schweikhardt.net> - 2022-01-02 13:28 +0000

#164114 — A question about style

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.

[toc] | [next] | [standalone]

#164117

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. 

[toc] | [prev] | [next] | [standalone]

#164118

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.

[toc] | [prev] | [next] | [standalone]

#164121

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"

[toc] | [prev] | [next] | [standalone]

#164122

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

[toc] | [prev] | [next] | [standalone]

#164125

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

[toc] | [prev] | [next] | [standalone]

#164127

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

[toc] | [prev] | [next] | [standalone]

#164128

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

[toc] | [prev] | [next] | [standalone]

#164129

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.

[toc] | [prev] | [next] | [standalone]

#164130

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

[toc] | [prev] | [next] | [standalone]

#164139

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

[toc] | [prev] | [next] | [standalone]

#164134

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.

[toc] | [prev] | [next] | [standalone]

#164140

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

[toc] | [prev] | [next] | [standalone]

#164136

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

[toc] | [prev] | [next] | [standalone]

#164220

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)

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c

csiph-web