Tag: pimp my code

char szPassword[255];
GenerateRandomPassword(szPassword, 8);
char szText[255];
sprintf(szText, "User: %s, ID: %d, Password: %s", szUser, int(userid++), szPassword);

Don’t increment and use a variable on the same line. We know, you’re very tricky, you saved a line. You also made sure that beginners to C++ don’t know what the result will be. Keep it simple stupid. Create the simplest most readable code possible, it makes skimming over code and debugging code much easier. Fixed length arrays are very prone to buffer overruns, in this example szPassword is probably only 8 characters long after calling GenerateRandomPassword, but szUser could be any length and could definitely overrun 255 characters. The best way to mitigate this problem is to use a real string class such as std::string. We can also avoid using sprintf by using a type safe string writing class, std::ostringstream. Code using std::ostringstream is also slightly more human readable.

userid++;
const std::string sPassword = GenerateRandomPassword(8);
std::ostringstream oText;
oText<<"User: "<<sUser<<", ID: "<<userid<<", Password: "<<sPassword;
std::string sText = oText.str();

There, type safe, buffer overflow safe, future proof and slightly more readable, what’s not to like?

Not exactly a redux, but very similar to last time:

bIsNotEmpty = false;
if (vNests.GetSize() != 0) {
  if (vNests[0]->vEggs.GetSize() != 0) bIsNotEmpty = true;
}

First of all we don’t actually care about the size, we just care that we have (Or don’t have) a nest with eggs in it. Depending on the container GetSize may or may not be a variable look up. IsEmpty is always a variable lookup:

bIsNotEmpty = false;
if (!vNests.IsEmpty()) {
  if (!vNests[0]->vEggs.IsEmpty()) bIsNotEmpty = true;
}

We can combine this into a single line:

bIsNotEmpty = (!vNests.IsEmpty() && !vNests[0]->vEggs.IsEmpty());

I would use more descriptive variable naming change my logic to use bIsEmpty/!bIsEmpty. Using bIsNot variables is often confusing and leads to harder to read code:

bIsNestWithEggs = (!vNests.IsEmpty() && !vNests[0]->vEggs.IsEmpty());
bIsEmpty = !bIsNestWithEggs;
// Use bIsEmpty and !bIsEmpty from now on

inline bool IsSpecial(const char* szValue)
{
 // Returns true if this is a special value
 if (stricmp(szValue, "MySpecialValue") == 0) {
   return true;
 }
 
 return false;
}

I see this sort of thing all the time. For boolean functions that call boolean functions the if and return statements are usually superflous, we can use the return value of (stricmp(szValue, “MySpecialValue”) == 0) itself:

// Returns true if this is a special value
inline bool IsSpecial(const char* szValue)
{
 return (stricmp(szValue, "MySpecialValue") == 0);
}

If it were up to me I would also use a string class and keep as much of the code as possible in the string “realm” (This makes the code a lot simpler and easier to read):

// Returns true if this is a special value
inline bool IsSpecial(const string& sValue)
{
 return (sValue == "MySpecialValue");
}