code readability

[A Haunted Army]

hey, just looking for a bit of advice on code readability, i've figured out a new and i hope better way to handle collisions but the when reviewing the code it all just blurrs together for me so is there some format teckneuqies i can use so its easier for me to read?

void b_Physics::Collision(const s_Polygon& Obstruct)
{
    //store the new delta time to calculate the new speed
    float Ntime = 0.0f;

    //copy of this objects polygon for growing, only want to modify the speed
    //not the polygon itself
    s_Polygon Polygon = this->Object.Polygon;

    //grow the copy of polygon in the direction that the object is moving then
    //find the time of collision, move the object speed back in time so the object
    //moves to the point of collision.
    //
    //to calculate the new speed i first need to get the time:
    //polygon - object / speed or object - polygon / speed gets the time, 
    //which one depends on which returns a positive number because time can never be negative.
    //
    //then:
    //object - polygon / time gets the new speed, positive or negative doesn't matter
    //because it will return the correct speed depending on the direction, left or up
    //will be negative and to move left or up needs a negative result.
    //right or down will be positive and to move right or down needs a positive result

    switch(this->Object.Motion.HorizDir){
        case s_Motion::LEFT  : Polygon.MinX = Polygon.MinX + this->Object.Motion.Xvel; break;
        case s_Motion::RIGHT : Polygon.MaxX = Polygon.MaxX + this->Object.Motion.Xvel; break;
        default : break;
    }

    if(this->Object.Polygon.MinY < Obstruct.MaxY && this->Object.Polygon.MaxY > Obstruct.MinY){
        if(this->Object.Motion.HorizDir == s_Motion::LEFT && Polygon.MinX < Obstruct.MaxX){
            Ntime = this->Object.Polygon.MinX - Obstruct.MaxX / this->Object.Motion.Xvel;
            this->Object.Motion.Xvel = Obstruct.MaxX - this->Object.Polygon.MinX / Ntime;

        }else if(this->Object.Motion.HorizDir == s_Motion::RIGHT && Polygon.MaxX > Obstruct.MinX){
            Ntime = Obstruct.MinX - this->Object.Polygon.MaxX / this->Object.Motion.Xvel;
            this->Object.Motion.Xvel = Obstruct.MinX - this->Object.Polygon.MaxX / Ntime;
        }
    }

    switch(this->Object.Motion.VerticDir){
        case s_Motion::UP   : Polygon.MinY = Polygon.MinY + this->Object.Motion.Yvel; break;
        case s_Motion::DOWN : Polygon.MaxY = Polygon.MaxY + this->Object.Motion.Yvel; break;
        default : break;
    }

    if(this->Object.Polygon.MinX < Obstruct.MaxX && this->Object.Polygon.MaxX > Obstruct.MinX){
        if(this->Object.Motion.VerticDir == s_Motion::UP && Polygon.MinY < Obstruct.MaxY){
            Ntime = this->Object.Polygon.MinY - Obstruct.MaxY / this->Object.Motion.Yvel;
            this->Object.Motion.Yvel = Obstruct.MaxY - this->Object.Polygon.MinY / Ntime;

        }else if(this->Object.Motion.VerticDir == s_Motion::DOWN && Polygon.MaxY > Obstruct.MinY){
            Ntime = Obstruct.MinY - this->Object.Polygon.MaxY / this->Object.Motion.Yvel;
            this->Object.Motion.Yvel = Obstruct.MaxY - this->Object.Polygon.MinY / Ntime;
        }
    }
}

thats the function, its all one big blurr for my eyes and i have to strain them to read it.

[NathanOliver]

This is how I would format your code.

void b_Physics::Collision(const s_Polygon& Obstruct)
{
    float Ntime = 0.0f;

    s_Polygon Polygon = this->Object.Polygon;

    switch(this->Object.Motion.HorizDir)
    {
        case s_Motion::LEFT: 
            Polygon.MinX = Polygon.MinX + this->Object.Motion.Xvel; 
            break;
        case s_Motion::RIGHT: 
            Polygon.MaxX = Polygon.MaxX + this->Object.Motion.Xvel; 
            break;
        default : break;
    }

    if(this->Object.Polygon.MinY < Obstruct.MaxY && this->Object.Polygon.MaxY > Obstruct.MinY)
    {
        if(this->Object.Motion.HorizDir == s_Motion::LEFT && Polygon.MinX < Obstruct.MaxX)
        {
            Ntime = this->Object.Polygon.MinX - Obstruct.MaxX / this->Object.Motion.Xvel;
            this->Object.Motion.Xvel = Obstruct.MaxX - this->Object.Polygon.MinX / Ntime;

        }
        else if(this->Object.Motion.HorizDir == s_Motion::RIGHT && Polygon.MaxX > Obstruct.MinX)
        {
            Ntime = Obstruct.MinX - this->Object.Polygon.MaxX / this->Object.Motion.Xvel;
            this->Object.Motion.Xvel = Obstruct.MinX - this->Object.Polygon.MaxX / Ntime;
        }
    }

    switch(this->Object.Motion.VerticDir)
    {
        case s_Motion::UP:
            Polygon.MinY = Polygon.MinY + this->Object.Motion.Yvel; 
            break;
        case s_Motion::DOWN: 
            Polygon.MaxY = Polygon.MaxY + this->Object.Motion.Yvel; 
            break;
        default : break;
    }

    if(this->Object.Polygon.MinX < Obstruct.MaxX && this->Object.Polygon.MaxX > Obstruct.MinX)
    {
        if(this->Object.Motion.VerticDir == s_Motion::UP && Polygon.MinY < Obstruct.MaxY)
        {
            Ntime = this->Object.Polygon.MinY - Obstruct.MaxY / this->Object.Motion.Yvel;
            this->Object.Motion.Yvel = Obstruct.MaxY - this->Object.Polygon.MinY / Ntime;

        }
        else if(this->Object.Motion.VerticDir == s_Motion::DOWN && Polygon.MaxY > Obstruct.MinY)
        {
            Ntime = Obstruct.MinY - this->Object.Polygon.MaxY / this->Object.Motion.Yvel;
            this->Object.Motion.Yvel = Obstruct.MaxY - this->Object.Polygon.MinY / Ntime;
        }
    }
}

[A Haunted Army]

hm.. thanks, that definitely is easier on the eyes, though personally i prefer simple switch statements on one line, saves space and is easy to follow.

but in general, is there any good guidlines i should... or could follow? i'm just a newb hobbiest and would prefer to get into good habbits sooner rather then later.

[mike_2000_17]

That's a nice question. Code readability is really important.

Here are a few tips:

1) Leave the explanation of what a function does outside the function, and the comments about how it is done inside the function, near the code that does it.

2) Use meaningful and complete names for the variables and classes.

In your code, I see a lot of names like "Object", "Polygon", "Physics", "Motion", "Ntime", "Yvel", etc... which are either too general to be meaningful or too short to the readable. As the saying goes, typing is cheap, reading is expensive. In other words, the time you spare by typing shorter names or by not taking the time to come up with more meaningful names is time you're gonna lose many folds when you come back around trying to discypher what the code means.

3) Use verbs in function names.

A function is supposed to do something, and its name should reflect that. Your function is called "Collision". What is that supposed to mean? Is is checking for a collision? Is it (also) reacting to a collision (e.g., bounce the objects)? Is it computing a collision force? I don't know because I don't know your code (until reading the implementation), and your function's name is not helping me.

4) Let the code talk for itself.

If you follow tip 2 and 3, you'll find that code will read like prose (sentences). For example, here's a line of code from some of my own code (from some CanadArm2 inverse kinematics code):

double max_wrist_to_wrist_dist = link_lengths[2] + link_lengths[3];

Pretty clear, no? The line means "the maximum wrist-to-wrist distance is equal to the lengths of the two middle links". There is no need for a comment line to say that because it is already clear from the code itself, which almost reads as easily as a sentence explaining it. By contrast, if you don't use meaningful names, you end up having to explain it in a comment, as so:

// Compute the maximum wrist-to-wrist distance by adding the lengths
// of the two middle links.
double m = ll[2] + ll[3];

That's very ugly, and that's going to be needed at every invocation of these meaninglessly-named variables. So, what do you prefer? Meaningful (and long) names and no need for much commenting, or short and un-expressive names and comments all over your code because nothing makes sense without them.

With functions (especially member functions), the same principle applies, if you use verbs, expressive names and especially if you keep the call-context in mind when you come up with those names, it will result in very clear statements in the code. Something like this:

FirstGeometry->checkCollisionWith( SecondGeometry );

Once again, no need for comments when the code is clear.

5) There is no competition for lowest LOC count.

In your code, you have lines like:

case s_Motion::UP   : Polygon.MinY = Polygon.MinY + this->Object.Motion.Yvel; break;

What's the point of that? The reader of the code didn't ask to play "Where's Waldo", where the "Waldos" to find are the semi-colons buried in a long line. Just space it out:

case s_Motion::UP : 
    Polygon.MinY = Polygon.MinY + this->Object.Motion.Yvel; 
    break;

6) Don't be afraid to create lots of local variables, including references.

Within a function, the compiler is perfectly capable of optimizing away trivial local variables, so you don't lose any performance by reusing the same variable to store different things at different times. Just create a new local variable for the new purpose, and let the compiler worry about optimization (DU-chain optimization).

On the same topic, references can be a nice way to reduce ugly and long chains of member addressing. For example, your code constantly uses the objects this->Object.Polygon and this->Object.Motion. Adding a couple of lines at the start of the function can go a long way:

s_Polygon& MyShape = this->Object.Polygon;
s_Motion& MyMotion = this->Object.Motion;

And then just use MyShape and MyMotion the rest of the way. Don't worry about performance, these references will most likely be optimized away.

7) Avoid large sections of comments that goes ahead of the code that it is commenting.

In your code, you have one big comment-section that rambles on about how the whole function works, and then you have all of the function's code. This means that as I read the code, if there is something I don't understand, I have to go back to the comment section, find the point in the comments that talks about the piece of code I had trouble with, and then go back and forth until I understand, before I can move on and repeat the same process at the next stumbling block. That's terrible. Comments are there to clarify what the code does, they must appear very near to where the clarification is needed.

I know that many people like to write a little paragraph of comment for themselves to gather their thoughts about how to write the function before they write it (i.e., write a "plan"). I often do that too, but I always split up that "plan" into separate lines for separate steps (which often turns into "pseudo-code") and then write the code directly between those lines of comment. The end result is much better than if you leave a big comment section at the beginning of the function.

8) Comments should either summarize a small section of code or remark on something that might not be obvious from a particular line of code (e.g., hidden assumption, etc.).

This tip is pretty simple. There's no point in stating the obvious. You don't want things like:

++i; // incrementing the 'i' counter.

Only pair a line of code with a comment-lines when you need to point out something that might not be obvious, like a hidden assumption. Something like:

double factor = value / divider;  // NOTE: 'divider' cannot be zero because of an earlier test (see above).

As you see, the comment does its job. You don't need to say that you are doing a division, but it's good to point out why it is OK not to test for a divide-by-zero condition here (because of some earlier condition-test that guarantees that). Once you follow that guideline, there won't be all that many reasons to put comments, except as a simple one-line (or two) introduction to what a little section (5-10 lines) of code does.

...

There are probably more tips, but that's a good start. Following those guidelines, your code transforms into:

/**
 * This function checks for a collision with the given geometry by growing 
 * the geometry in the direction of motion and solving for a collision time.
 * If a collision is detected, the object is moved back in time to the point 
 * of collision and a new speed is calculated for post-collision motion.
 */
void b_Physics::respondToCollisionWith(const s_Polygon& Obstacle)
{
    s_Motion&  ThisMotion     = this->Object.Motion;
    s_Polygon& ThisPolygon    = this->Object.Polygon;
    s_Polygon  GrowingPolygon = this->Object.Polygon; // NOTE: local copy.

    // Grow the geometry in horizontal direction:
    switch(this->Object.Motion.HorizDir) {
        case s_Motion::LEFT : 
            GrowingPolygon.MinX = GrowingPolygon.MinX + ThisMotion.Xvel; 
            break;
        case s_Motion::RIGHT : 
            GrowingPolygon.MaxX = GrowingPolygon.MaxX + ThisMotion.Xvel; 
            break;
        default : 
          break;
    }

    // Check for overlap on along y-axis for the original geometries:
    if( (ThisPolygon.MinY < Obstacle.MaxY) && 
        (ThisPolygon.MaxY > Obstacle.MinY) ) {

        // Then, check if there is collision between grown geometry and obstacle:
        if( (ThisMotion.HorizDir == s_Motion::LEFT) && 
            (GrowingPolygon.MinX < Obstacle.MaxX) ) {

            float Ntime = ThisPolygon.MinX - Obstacle.MaxX / ThisMotion.Xvel;
            ThisMotion.Xvel = Obstacle.MaxX - ThisPolygon.MinX / Ntime;

        } else if( (ThisMotion.HorizDir == s_Motion::RIGHT) && 
                  (GrowingPolygon.MaxX > Obstacle.MinX) ) {

            float Ntime = Obstacle.MinX - ThisPolygon.MaxX / ThisMotion.Xvel;
            ThisMotion.Xvel = Obstacle.MinX - ThisPolygon.MaxX / Ntime;

        }
    }

    // Grow the geometry in vertical direction:
    switch(ThisMotion.VerticDir) {
        case s_Motion::UP : 
            GrowingPolygon.MinY = GrowingPolygon.MinY + ThisMotion.Yvel; 
            break;
        case s_Motion::DOWN : 
            GrowingPolygon.MaxY = GrowingPolygon.MaxY + ThisMotion.Yvel; 
            break;
        default : 
            break;
    }

    // Check for overlap on along x-axis for the original geometries:
    if( (ThisPolygon.MinX < Obstacle.MaxX) && 
        (ThisPolygon.MaxX > Obstacle.MinX) ) {

        // Then, check if there is collision between grown geometry and obstacle:
        if( (ThisMotion.VerticDir == s_Motion::UP) && 
            (GrowingPolygon.MinY < Obstacle.MaxY) ) {

            float Ntime = ThisPolygon.MinY - Obstacle.MaxY / ThisMotion.Yvel;
            ThisMotion.Yvel = Obstacle.MaxY - ThisPolygon.MinY / Ntime;

        } else if( (ThisMotion.VerticDir == s_Motion::DOWN) && 
                   (GrowingPolygon.MaxY > Obstacle.MinY) ) {

            float Ntime = Obstacle.MinY - ThisPolygon.MaxY / ThisMotion.Yvel;
            ThisMotion.Yvel = Obstacle.MaxY - ThisPolygon.MinY / Ntime;

        }
    }
}

Better, isn't it?

[A Haunted Army]

wow thanks, that's going to help alot!

[deceptikon]

1) Leave the explanation of what a function does outside the function, and the comments about how it is done inside the function, near the code that does it.

I'd go even further (since I'm a sparse commenter) and say that comments should be limited to the following (examples taken from here, and recognizing that there's some redundancy with your recommendations):

• Do comment why the code is there and why it works. This can be a business-level explanation of the problem that's being solved or a summary of the following block of code that saves the reader the trouble of deciphering what's happening and making an assumption about the intended purpose. Two examples:

  /* Strip leading whitespace */
  while (isspace(*it))
      ++it;
  
  /* Check for an integer part sign */
  if (*it == '-' || *it == '+')
      sign = (*it++ == '-');
  

  Notice how the code is reasonably clear without the comments, but the comments really nail down what the author was doing with each block.

• Do comment how the code is intended to be used. This is in reference to things like Doxygen comments that are useful in generating documentation as well as providing code-level documentation.

• Do include things that are going through your mind as you write the code. The reader won't necessarily be as intimate as you are with the problem and the solution, so there may be confusion if you're not expressing it. Two examples of comments that highlight what the author was thinking:

  if (base == 16) {
      /*
          Since we have the bit values for an IEEE 754 double, we 
          might as well use them to build the value directly.
      */
      unsigned long long mantissa = fpart;
      unsigned long long exponent = epart + (DBL_MAX_EXP - 1);
      _real8_t fpv;
  
      /* fpart needs to be in the most significant bit position */
      while ((mantissa & (1ULL << 51)) == 0)
          mantissa <<= 1;
  
      fpv.parts.sign = sign;
      fpv.parts.mantissa = mantissa;
      fpv.parts.exponent = exponent;
  
      return fpv.fvalue;
  }
  

When not to comment:

• Don't describe how the code works unless it's especially obtuse. The code itself tells you how it works; comments are not subtitles. If it's really so unreadable that you need to comment how it works, consider rewriting it to be clearer.

• Don't version your code with comments. That's what version control software is for.

• Don't state the obvious.

Granted, it's a fine line between a good comment and a bad comment. I wouldn't expect everyone to have perfect and beautiful comments all of the time, especially since I most certainly don't. But it's important to remember why comments are there, who they're there for, and strive to make them as meaningful as possible with as little fluff as possible.

Overly commented code is just as annoying to read as completely uncommented code. ;)

5) There is no competition for lowest LOC count.

Line of count count? ;) Joking aside, while there's no competition going on, there's benefit to limiting your use of vertical space. It's easier to read a block of code that fits on the screen than having to scroll to get the whole picture and potentially miss something that moved off the edge.

Obviously you can go too far with that, but I think there's a happy medium between being excessively concise and using line breaks as if they were an orgasm button.

With that said, I like to use the one line case statement as well. However, I try to limit its use to situations where all but the default case are one line, and if any of them aren't then they're all spread out over several lines as in your example. The idea is that if all cases are clear and consistent with the single line approach, it's viable, otherwise it's not.

As an example from here, I think the compressed cases are more readable in the given situation:

switch (spec.type) {
case _SPEC_UCHAR:    *va_arg(args, unsigned char*) = (unsigned char)value;   break;
case _SPEC_USHORT:   *va_arg(args, unsigned short*) = (unsigned short)value; break;
case _SPEC_UINT:     *va_arg(args, unsigned int*) = (unsigned int)value;     break;
case _SPEC_ULONG:    *va_arg(args, unsigned long*) = (unsigned long)value;   break;
case _SPEC_ULLONG:   /* Fall through */
case _SPEC_UINTMAXT: *va_arg(args, unsigned long long*) = value;             break;
case _SPEC_SIZET:    *va_arg(args, unsigned int*) = (unsigned int)value;     break;
}

Than if they were uncompressed:

switch (spec.type) {
case _SPEC_UCHAR:    
    *va_arg(args, unsigned char*) = (unsigned char)value;   
    break;
case _SPEC_USHORT:   
    *va_arg(args, unsigned short*) = (unsigned short)value; 
    break;
case _SPEC_UINT:     
    *va_arg(args, unsigned int*) = (unsigned int)value;     
    break;
case _SPEC_ULONG:    
    *va_arg(args, unsigned long*) = (unsigned long)value;   
    break;
case _SPEC_ULLONG:   
    /* Fall through */
case _SPEC_UINTMAXT: 
    *va_arg(args, unsigned long long*) = value;             
    break;
case _SPEC_SIZET:    
    *va_arg(args, unsigned int*) = (unsigned int)value;     
    break;
}

6) Don't be afraid to create lots of local variables, including references.

But take note that humans can keep on average 7 different things in their head at one time. The more variables you use, the harder it is to mentally keep track of them all when reading the code.

[A Haunted Army]

big thanks, been going through and adjusting my code around your advice and its become a lot more readable and followable.

just a quick question, should nested switch statements be avoided? i haven't done nested switch statements yet but i'm thinking about seperating some of the code into functions and nested switch statements come to mind for the main handling.

[mike_2000_17]

Well, I have a great deal of distaste for switch statements overall, in part because I find the syntax ugly, and in part because they are often an indicator that there's something you are missing out on an opportunity to have a more flexible design. In other words, when you leverage polymorphism well (statically or dynamically), you don't really need to use switch statements all that much. But with that said, there are definitely many uses for switch statements (some are necessary, others are just more quick and convenient than a more "designed" alternative). And my personal taste in the matter does not amount to a coding guideline, for sure, so, use them if you want to.

As for nested switch statements, I don't see any problems with that, as long as it is properly spaced out and indented. And obviously, you don't want redundancy (repeating a very similar nested switch statement again and again), if that's the case, consider rethinking it to see if it is possible to avoid that (e.g., putting the nested code into a separate function, or using dynamic polymorphism).

But take note that humans can keep on average 7 different things in their head at one time. The more variables you use, the harder it is to mentally keep track of them all when reading the code.

Yes, that's a good point and common-sense should always apply whenever following any kind of coding guidelines (whether stylistic or about code correctness). In my experience, however, individual functions rarely need to be very long (or contain lots of variables), and if a function is very long it's often an indication that some of the steps could be split into sub-functions (and possibly reused within the function or elsewhere). One exception I commonly encounter is when writing a large mathematical function (e.g., geometric calculations) which can often end up being quite large and difficult to track because they are based on some large and complicated mathematical derivation that is very specific to the problem at hand, but often in this case, you need a fairly heavy amount of comments to make sense of the code.

Just to clarify that point about creating lots of local variables, what I really had in mind were some rather terrible things that I've seen before. A typical example in a numerical calculation is that you define a double variable for some purpose, and then half-way through the algorithm you need another double variable and then you get a genius idea "hey, I don't need that earlier variable anymore (for the rest of the function), so, I'll just re-use it instead of creating a new one". Doing this can lead to some pretty terrible readability (either the variable name doesn't match its purpose at one point or another, or you are forced to use a generic name like a), and possibly a bug if later someone adds something towards the end of the function that assumes the variable still caries its original purpose. That's why variables should be kept as localized as possible (within the narrowest scope in which they are needed). I'm especially annoyed by these kinds of long numerical calculations with short (one letter!) names for variables and stupid recycling of variables, mostly because a lot of the important (classic) and complicated numerical code come from old Fortran or C implementations where those kinds of atrocities were common practice and sometimes needed (i.e., 80 character limit without being able to continue on the next line). The torture that it is to try and read that kind of code has left its scars on me.

And about references, they are often called "aliases" for variables / objects, and it's good to keep that very basic concept in mind, i.e., references are not just for pass-by-reference, you can also use them locally, within a function, to re-name a variable. As in the example given earlier, if an often-used variable is "hard to reach" (e.g., a data member of a data member of a data member of an object), then just re-name it to something that is shorter and makes more sense within the context of the function.

[A Haunted Army]

thanks, i think i'll rethink this a bit then because it would have ended up with a few repeated nested switches or if-elses. i think i'll make a few inline math functions and re-use those. think i'll put them outside the class as well.

i do sort of agree with your distaste towards switch statements, when theres quite a bit of code going on switch statements makes things confusing which is why i choose if statements in such situations. if its simple then i use switch statements.

i already knew how referencing worked, what i didn't know was that compiler would optimize so i thought i just wasn't worth the extra proccessing time. i found that gnu gcc had optimization off by default so i've set it to full optimzation.