Jump to content

  • Log In with Google      Sign In   
  • Create Account

My programming style


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • This topic is locked This topic is locked
43 replies to this topic

#1 invutil   Crossbones+   -  Reputation: 1932

Posted 25 April 2013 - 11:47 PM

Share a few lines of code that exemplify your programming style. Share innovations.

 

I'll start.

 

void UpdateAI()
{
   CPlayer* p;
 
    for(int i=0; i<PLAYERS; i++)
    {
        p = &g_player[i];
 
        if(!p->on)
            continue;
 
        if(!p->ai)
            continue;
 
        UpdateAI(p);
    }
}

 

Features:

 

- Use pointers with one, first letter of word of type (CPlayer* p, CUnit* u, CUnitType* t, CCamera* c, CClient* c, CEntity* e, CPacket* p) or purpose (position, radius)

- Use this pointer instead of fully qualified variable reference
- Use a single line for each continue if-statement condition
- Part object-oriented, part procedure-oriented

 

Another example:

 

bool CollidesWithUnits(float x, float z, float hwx, float hwz, bool isUnit, CUnit* thisU, CUnit* ignore, bool checkPass)
{
  CUnit* u;
  CUnitType* t;
  float r;
  CVector3 p;
  CCamera* c;

  for(int i=0; i<UNITS; i++)
  {    
    u = &g_unit[i]; // <--

    if(!u->on)
      continue;

    if(u == ignore)
      continue;

    if(u == thisU)
      continue;

    t = &g_unitType[u->type]; // <--

    //...

    c = &u->camera; // <--
    p = c->Position(); // <--
    r = t->radius; // <--

    //...
  }

  return false;
}

 

- Multiple levels of pointers retrieving something from other pointers

 

Instead of nesting if-statements,

 

bool CWidget::DropDown_prelbuttonup()
{
  if(ldown)
  {
    ldown = false;

    if(opened)
    {
      if(mousescroll)
      {
        mousescroll = false;
        return true;  // intercept mouse event
      }

      for(...)
      {
        // list item?
        if(...)
        {
          //....
          opened = false;
          return true;  // intercept mouse event
        }
      }

      // up button?
      if(...)
      {
        //...
        return true;
      }

      // down button?
      if(...)
      {
        //...
        return true;
      }

      opened = false;

      return true;  // intercept mouse event
    }

    return false;
  }

  return false;
}

 

I try to write

if(!...)
  return;

 

 

whenever I can, so that it looks like this:

 

bool CWidget::DropDown_prelbuttonup()
{
  if(!ldown)
    return false;
  
  ldown = false;

  if(!opened)
    return false;

  if(mousescroll)
  {
    mousescroll = false;
    return true;  // intercept mouse event
  }

  for(...)
  {
    // list item?
    if(...)
    {
      //....
      opened = false;
      return true;  // intercept mouse event
    }
  }

  // up button?
  if(...)
  {
    //...
    return true;
  }

  // down button?
  if(...)
  {
    //...
    return true;
  }

  opened = false;

  return true;  // intercept mouse event
}

 

This was a big paradigm shift for me, when I first thought of this. But now I do it all the time.


Edited by polyfrag, 25 April 2013 - 11:50 PM.


Sponsor:

#2 mhagain   Crossbones+   -  Reputation: 8286

Posted 26 April 2013 - 04:47 AM

One critique.  When declaring a pointer I get mental alarm bells if I don't see it initialized, so instead of:

 

CPlayer* p;

 

I prefer to see:

 

CPlayer* p = NULL; // Substitute with nullptr, 0, or whatever according to your preference/standard-followed/etc

 

This applies even if the rest of the code logic makes it impossible for the pointer to be uninitialized at the time it's first used; it may be impossible now but who's to say that it's going to remain impossible following months/years of maintenance?


It appears that the gentleman thought C++ was extremely difficult and he was overjoyed that the machine was absorbing it; he understood that good C++ is difficult but the best C++ is well-nigh unintelligible.


#3 rip-off   Moderators   -  Reputation: 8764

Posted 26 April 2013 - 05:59 AM

There is almost universal consensus that abbreviating variable names is not a good idea. With minor exceptions for well understood idioms, such as loop counters, variables should be given descriptive names.

 

The type is the poorest name, as that is the one thing that we know for certain. The intended purpose is the least certain. Hopefully it is made obvious in the code that follows, but when that code contains a bug (or even just looks like it might), there is ambiguity between the desired and actual behaviour. We cannot rely 100% on the code to infer the intent.

 

For example, in your CollidesWithUnits() function, the parameters "hwx" and "hwz" and "checkPass" are obscure. Lacking other information, I would presume "x", "z" are a position of some sort. The parameter "isUnit" is surprising because the types involved seem to imply that these are all units. I presume "thisU" is the "current" unit, and you are trying to exclude self collisions. Maybe isUnit is related to thisU being perhaps optional? It is clear what "ignore" is doing, but it is not clear why there is some other unit being excluded from the collision.

 

Maybe with the full source of the function I would be able to infer the purpose of the parameters, both in the function itself and in the overall scope of the game.

 

Have you considered declaring your variables on the line they are first used? It would avoid the problem of having an uninitialised variable.



#4 mhagain   Crossbones+   -  Reputation: 8286

Posted 26 April 2013 - 06:56 AM

This block:

 

  for(...)
  {
    // list item?
    if(...)
    {
      //....
      opened = false;
      return true;  // intercept mouse event
    }
  }

 

Suggests usage of the for...if anti-pattern.  Now, it may not be, it probably isn't, but it is something to be aware of and watch out for.

 

rip-off is correct about this:

 

Have you considered declaring your variables on the line they are first used? It would avoid the problem of having an uninitialised variable.

 

It's a good habit to get into, and also has the good practice of limiting your variable scope to the narrowest possible.  If your first block of code, "p" does not need to exist outside of the loop, so it shouldn't.  This kind of basic stuff will help make your code more robust too.


Edited by mhagain, 26 April 2013 - 06:57 AM.

It appears that the gentleman thought C++ was extremely difficult and he was overjoyed that the machine was absorbing it; he understood that good C++ is difficult but the best C++ is well-nigh unintelligible.


#5 phantom   Moderators   -  Reputation: 7596

Posted 26 April 2013 - 07:02 AM

I saw 'CClassName'.
I stopped reading.

Dumbest naming convention ever and I hate everyone who does it.

#6 Bearhugger   Members   -  Reputation: 581

Posted 26 April 2013 - 08:02 AM

There is almost universal consensus that abbreviating variable names is not a good idea. With minor exceptions for well understood idioms, such as loop counters, variables should be given descriptive names.

I have to disagree with that. One-letter variable names are fine if they are in a very small scope, because you can't forget what they are and what they're initialized to when they're declared just 2~3 lines above.

// this is fine
if (this->SomeCondition())
{
    SOME_DATA_STRUCT* d = this->GetDataStruct();
    this->DoThis(d);
    this->DoThat(d);
}


// but this is not fine
void MyClass::VeryLongMethod()
{
    const SOME_DATA_STRUCT* d = this->GetDataStruct();

    //
    // Lots of lines of code go here.
    //

    // WTF was d again!?
    this->DoThis(d);
    this->DoThat(d);
}


#7 mhagain   Crossbones+   -  Reputation: 8286

Posted 26 April 2013 - 08:19 AM

// this is fine
if (this->SomeCondition())
{
    SOME_DATA_STRUCT* d = this->GetDataStruct();
    this->DoThis(d);
    this->DoThat(d);
}

 

This is fine indeed - until a coupla years later you come back to the code and find that maintenance/evolution/whatever has added 200 extra lines of code between your "SOME_DATA_STRUCT* d = this->GetDataStruct();" and your "this->DoThis(d);".  Then it stops being fine and you'll wish you'd given it a better name first time round.

 

That's kinda the point here.  Code isn't static; you don't just write it once and then leave it be forever more, and even if you think you will, trust me - you won't.  So you always write with the assumption that code is going to be revisited, worked over, bugfixed, have functionality added, and generally sprout all kinds of interesting tentacles in all kinds of interesting directions during it's lifetime, and that way you're protected when (and it is a "when", not an "if") that time does (and it is a "does", not a "may") come.

 

At the very least pick a name such that doing a search for all occurrances of that name in your codebase will give you what is genuinely all occurrances and nothing else.


Edited by mhagain, 26 April 2013 - 08:21 AM.

It appears that the gentleman thought C++ was extremely difficult and he was overjoyed that the machine was absorbing it; he understood that good C++ is difficult but the best C++ is well-nigh unintelligible.


#8 kunos   Crossbones+   -  Reputation: 2207

Posted 26 April 2013 - 09:24 AM

I saw 'CClassName'.
I stopped reading.

Dumbest naming convention ever and I hate everyone who does it.

 

I agree.. but I didnt stop reading and found more than that :P

 

Seriously.. I dont understand the point of this thread.. the code posted is nothing revolutionary and, from so many points of view, obsolete, error prone and verbose.. I mean... why 2 if that end up in a continue? why oh why is this supposed to be any more clear than a boolean operation? Raw arrays everywhere with no sign of bounds check.. why would I want to declare a variable faaaaar away from where it is actually first assigned? anyway.. good luck :P


Stefano Casillo
Lead Programmer
TWITTER: @KunosStefano
AssettoCorsa - netKar PRO - Kunos Simulazioni

#9 BeerNutts   Crossbones+   -  Reputation: 3018

Posted 26 April 2013 - 09:43 AM

void UpdateAI()
{
  for(int i = 0; i < PlayerList.size(); ++i)
  {
    if(!PlayerList[i].IsAlive || !PlayerList[i].IsAi) {
      continue;
    }
    UpdateAI(PlayerList[i]);
  }
}
 
void UpdateAI(TPlayer& aiPlayer);

My Gamedev Journal: 2D Game Making, the Easy Way

---(Old Blog, still has good info): 2dGameMaking
-----
"No one ever posts on that message board; it's too crowded." - Yoga Berra (sorta)

#10 BornToCode   Members   -  Reputation: 951

Posted 26 April 2013 - 10:35 AM

I saw 'CClassName'.
I stopped reading.

Dumbest naming convention ever and I hate everyone who does it.

I couldn't agree more. Another thing i found stupid is why people use a signle character for a variable name. That code is unreadable except by the person who wrote it in the first place.



#11 invutil   Crossbones+   -  Reputation: 1932

Posted 26 April 2013 - 07:39 PM

I think abbreviations are good as long as there are no name conflicts. After all, deque is an abbreviation. And the whole sockaddr struct is made of cryptic abbrevations. In my code I reuse the same abbreviations in dozens of functions and when they conflict (like player, powerline, pipeline) I use something a bit longer like pow, pipe. And it's perfectly readable. I got tired of writing so many utility functions that reuse the same variables that I started using single letter names. I reserve the full names ('player', 'unit', 'entity' etc.) for the associated integer ID index into the array.

hwx and hwz are the half-length of the bounding box on the x and z axises. checkPass is needed to specify if we want to check for collisions with pass-through units. I made labourers and trucks pass-through to speed up pathfinding where collision checks have to be done at each step of the path. I want to check for collisions with pass-through units though when they are spawned around the building that produced them. isUnit is needed because the function is also called when selecting a location to construct new buildings. Both checkPass and isUnit have to be true for it to check for collisions with pass-through units. thisU and "ignore" are needed because the function is used in pathfinding and the target might be the location of another unit, when attacking or when going to drive a truck.

I declare variables outside of loops because I have a superstition that the CPU will waste extra cycles reallocating the variable on each iteration of the loop.

The full code can be found at https://github.com/polyf/corpstates

I use the 'CClassName' convention because GameTutorials.com used it.

#12 Bacterius   Crossbones+   -  Reputation: 9305

Posted 26 April 2013 - 08:54 PM

I declare variables outside of loops because I have a superstition that the CPU will waste extra cycles reallocating the variable on each iteration of the loop.

 

Even if this was the case - it isn't - allocating a variable on the stack essentially consists of incrementing the stack pointer. It costs nothing.

 

Write your code for other people, not for the computer. It's the compiler's job to make your stuff run fast and you can optimize later if it's not doing a good job. But superstitions like these are often unfounded and lead to unusual idioms which can make the code difficult to follow.


The slowsort algorithm is a perfect illustration of the multiply and surrender paradigm, which is perhaps the single most important paradigm in the development of reluctant algorithms. The basic multiply and surrender strategy consists in replacing the problem at hand by two or more subproblems, each slightly simpler than the original, and continue multiplying subproblems and subsubproblems recursively in this fashion as long as possible. At some point the subproblems will all become so simple that their solution can no longer be postponed, and we will have to surrender. Experience shows that, in most cases, by the time this point is reached the total work will be substantially higher than what could have been wasted by a more direct approach.

 

- Pessimal Algorithms and Simplexity Analysis


#13 kunos   Crossbones+   -  Reputation: 2207

Posted 27 April 2013 - 12:03 AM

void UpdateAI()
{
  for(int i = 0; i < PlayerList.size(); ++i)
  {
    if(!PlayerList[i].IsAlive || !PlayerList[i].IsAi) {
      continue;
    }
    UpdateAI(PlayerList[i]);
  }
}
 
void UpdateAI(TPlayer& aiPlayer);

 

why the continue? Why the checks with ! ??? Why not the proper for (auto& player : playerList) ? And even better, if we really need to have a function that updates only one player (and I really feel we don't) why are the rules to call it expressed OUTSIDE? It should be as simple as calling a std::for_each(begin(playerList), end(playerList), UpdateAI) and then do the simple rule check INSIDE that if (player.IsActive && player.IsAI) { do your things} .. this opens the door to parallelism.. on Windows replace std:: with parallel:: and you're multithread.. and it's 3 lines of code! .. no dumb "!" to mess up your logic, no continue to early exit a loop.. it doesnt make sense at ALL! I code what I DO when a certain condition is met, thinking "right I don't do something and continue the loop if a couple of not conditions are true" is really on the edge of insanity.. it really is.

 

Should I continue? pun intended tongue.png ... do we need an UpdateAI ? do we need a IsAI? Chances are that there is also an Update somewhere for !IsAI players.. what ever happened to polymorphism???? A Player (please no CPlayer sad.png ) class with a virtual Update method and a PlayerAI class that overrides that Update.. 

 

for (auto player : playerList) player->Update();

 

It's 1 line of code!


Edited by kunos, 27 April 2013 - 12:04 AM.

Stefano Casillo
Lead Programmer
TWITTER: @KunosStefano
AssettoCorsa - netKar PRO - Kunos Simulazioni

#14 Alpha_ProgDes   Crossbones+   -  Reputation: 4692

Posted 27 April 2013 - 12:45 AM

I use the 'CClassName' convention because GameTutorials.com used it.

 

Holy Jeezbus! People still use that site?! I thought it died after the 90s!


Beginner in Game Development? Read here.
 
Super Mario Bros clone tutorial written in XNA 4.0 [MonoGame, ANX, and MonoXNA] by Scott Haley
 
If you have found any of the posts helpful, please show your appreciation by clicking the up arrow on those posts Posted Image
 
Spoiler

#15 invutil   Crossbones+   -  Reputation: 1932

Posted 27 April 2013 - 01:49 AM

kunos, what you wrote was too complex for me. And if I use polymorphism and make a PlayerAI class I can't have it in the same global array.

#16 invutil   Crossbones+   -  Reputation: 1932

Posted 27 April 2013 - 01:52 AM

Holy Jeezbus! People still use that site?! I thought it died after the 90s!


That's where I learned how to use Quake 3 BSP maps. Don't know any other good sources.

#17 Anthony Serrano   Members   -  Reputation: 1256

Posted 27 April 2013 - 04:49 AM

kunos, what you wrote was too complex for me. And if I use polymorphism and make a PlayerAI class I can't have it in the same global array.

You shouldn't be using arrays of objects in the first place - use an array of pointers to objects (or better yet a vector of pointers).  And it probably shouldn't be a global array, either.



#18 invutil   Crossbones+   -  Reputation: 1932

Posted 27 April 2013 - 04:55 AM

If I use an array/vector of pointers how do I hold pointers of different inherited classes? And, yes, it HAS to be global. That's just plain retarded.

#19 Mussi   Crossbones+   -  Reputation: 2107

Posted 27 April 2013 - 05:00 AM

If I use an array/vector of pointers how do I hold pointers of different inherited classes?

By using pointers to their common base class. Take a look here and here to get a better understanding of inheritance and polymorphism.



#20 mhagain   Crossbones+   -  Reputation: 8286

Posted 27 April 2013 - 01:53 PM

I declare variables outside of loops because I have a superstition that the CPU will waste extra cycles reallocating the variable on each iteration of the loop.

 

This is something you need to get out of your head real fast.  Instead of superstition you need to make decisions based on fact, and use tools available to you to inform yourself about what that fact is.  In this case, reading documentation, viewing disassembly and running benchmarks would soon enough tell you the truth.

 

Right now you're voodoo-optimizing, you're pre-emptively optimizing, you're trying to second-guess the compiler, you're assuming that the C++ you write is going to generate a line-for-line equivalent when compiled, and it's not even based on anything more solid than "superstition".  That can only lead to worse and worse habits over time, so cure yourself of it now before it does real damage to you.


It appears that the gentleman thought C++ was extremely difficult and he was overjoyed that the machine was absorbing it; he understood that good C++ is difficult but the best C++ is well-nigh unintelligible.





Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS