Sign in to follow this  
polyfrag

My programming style

Recommended Posts

polyfrag    2502

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

Share this post


Link to post
Share on other sites
mhagain    13430

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?

Share this post


Link to post
Share on other sites
rip-off    10976

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.

Share this post


Link to post
Share on other sites
mhagain    13430

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

Share this post


Link to post
Share on other sites
Bearhugger    1276

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

Share this post


Link to post
Share on other sites
mhagain    13430
// 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

Share this post


Link to post
Share on other sites
kunos    2254

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

Share this post


Link to post
Share on other sites
BeerNutts    4400

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

Share this post


Link to post
Share on other sites
BornToCode    1185

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.

Share this post


Link to post
Share on other sites
polyfrag    2502
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.

Share this post


Link to post
Share on other sites
Bacterius    13165

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.

Share this post


Link to post
Share on other sites
kunos    2254

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

Share this post


Link to post
Share on other sites
Anthony Serrano    3285

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.

Share this post


Link to post
Share on other sites
Mussi    4407

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.

Share this post


Link to post
Share on other sites
mhagain    13430

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.

Share this post


Link to post
Share on other sites
kunos    2254

And, yes, it HAS to be global. That's just plain retarded.

 

No it HAS NOT. You are missing the entire point of programming in any language above assembler: encapsulation. Even by keeping a procedural approach to the problem, wanna see how the global goes away? Here it is:

 

int main()

{

vector<Player*> players;

 

// CREATE 10 players

for (int i=0;i<10;i++)

{

players.push_back(new Player());
}

 

// GAME LOOP

 

while (true)

{

updatePlayers(players);

render(players);

}

 

// clean up yada yada

 

return 0;

}

 

void updatePlayers(vector<Player*>& players)

{

for (auto player : players)

{

// UPDATE THE PLAYER
}
}

 

there you go.. players is not global anymore and only functions that REQUIRE access to it receive it as a parameter.

This gives you the huge advantage that now you know which subset of your code can access to players.. and when your code reaches 100k lines of code possibly multithreaded, trust me, you'll be glad to be limited to 3-4 functions instead of the EVERYWHERE you get from a global.

Edited by kunos

Share this post


Link to post
Share on other sites
polyfrag    2502

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.

 

John Carmack does it toooooO

 

https://github.com/id-Software/Quake-III-Arena/blob/master/common/bspfile.c

 

void UnparseEntities( void ) {
	char	*buf, *end;
	epair_t	*ep; // <---- THIS
	char	line[2048];
	int		i;
	char	key[1024], value[1024];

	buf = dentdata;
	end = buf;
	*end = 0;

	for (i=0 ; i<num_entities ; i++) {
		ep = entities[i].epairs; // <------ HERE
		if ( !ep ) {
			continue;	// ent got removed
		}

		strcat (end,"{\n");
		end += 2;

		for ( ep = entities[i].epairs ; ep ; ep=ep->next ) {
			strcpy (key, ep->key);
			StripTrailing (key);
			strcpy (value, ep->value);
			StripTrailing (value);

			sprintf (line, "\"%s\" \"%s\"\n", key, value);
			strcat (end, line);
			end += strlen(line);
		}
		strcat (end,"}\n");
		end += 2;

		if (end > buf + MAX_MAP_ENTSTRING) {
			Error ("Entity text too long");
		}
	}
	entdatasize = end - buf + 1;
}

Share this post


Link to post
Share on other sites
_the_phantom_    11250
That's because he was writing C and in C (at least until maybe C99?) you HAD to declare all your variables up front and not on the line they were first used.

Secondly that is code from Quake3, a 14 year old game based on an even older engine, the state of the art has moved on ALOT since then... a lot.

Compilers are better, hardware is different, the world moves on; there is probably very little of worth coding style wise (and maybe even data structure wise) in code that old.

In short 'huhuhuh Carmack did it 14 years ago!' might not be voodoo optimisation instead you've ventured into the realm of cargo cult coding - learn to think for yourself or, frankly, gtfo of my profession.

Share this post


Link to post
Share on other sites
Guest
This topic is now closed to further replies.
Sign in to follow this