Title: If your going to use it, learn it!

I was recently asked if I could spot why this code was causing some "weird" results. The code is intended to parse the following configuration file:

#BOT USERINFO FILE 
#A # means a comment to the end of the line.
#As many entries as you wish can be added by editing this file.
#Format is: 
#"<bot number> <bot name> <bot model> <bot shirt color> <bot pants color>"
bot1 Onna onna 30 6
bot2 Hondo gordon 30 6
bot3 Dijmo gordon 30 6
bot4 Jack barney 150 150
bot5 Timmy gordon 30 6
bot6 Lisa femsci 30 6
bot7 Ryan barney 30 30
bot8 Johnny gordon 30 6
bot9 Jarvis gordon 30 6
bot10 Billy gordon 30 6

Which describes the attributes of some robots for a computer game. The code is supposed to be in C++ however the author is really a C programmer, and it was not really his choice to move to C++. I would like to use this code for two things, the first is a code review and critique of his approach, and the second is to look at true C++ alternatives. So onto his code:

void SetNameByNum (char *name, char *model, char *top, char *btm) {

The function passes pointers to buffers defined elsewhere for each of the attributes. A block of local variables are declared next with assorted magic numbers appearing without justification.

  char buffer;
  int charnum = 0;
  int norec;
  char filename[24];
  char bnumstr[4];
  char *readin = new char[8];
  char *comparer;
  FILE *f;

Next the file is opened with some error checking, which is not ideal as the game dir is specified by the client and can typically exceed the allocated space for filename.

  g_engfuncs.pfnGetGameDir(filename);
  sprintf(filename, "%s\\bots.cfg", filename);
  f = fopen(filename, "r");
  if (!f) return;

The next bit is the worst as it depends on the global variable numbots and worst of all on a non-standard function _itoa (as the games code base is commonly distributed on both Unix and Windows OS's).

  _itoa(numbots + 1, bnumstr, 10);

The next bit of code does the main parsing of the file. There are several things that I don't like about this code, firstly the way that the comments (in the data file) are handled with the repeated tests on the variable norec and I prefer conditional statements to be bracketed. I'm going to skip the rest of the code and allow you to have a look, see if you can spot the obvious mistakes.

  while (!feof(f)) {
    fscanf(f, "%c", &buffer);
    if (buffer == '#' && norec != 1) norec = 1;

    if (buffer == '\n')   norec = 0;

    if (buffer == 'b' && norec != 1) {     //Found what we want
      readin[charnum] = buffer;
      charnum++;
      fscanf(f, "%c", &buffer);
    }
    comparer = strstr(readin, bnumstr);
    if (comparer && strcmp(comparer, bnumstr) == 0) {
      fscanf(f, "%c", &buffer);
      charnum = 0;
      while (buffer != ' ') {  name[charnum] = buffer; charnum++;  fscanf(f, "%c", &buffer);  }

      fscanf(f, "%c", &buffer);  charnum = 0;

      while (buffer != ' ') { model[charnum] = buffer; charnum++;   fscanf(f, "%c", &buffer); }

      fscanf(f, "%c", &buffer);  charnum = 0;

      while (buffer != ' ') { top[charnum] = buffer;  charnum++; fscanf(f, "%c", &buffer);  }

      fscanf(f, "%c", &buffer);  charnum = 0;

      while (buffer != '\n') {  btm[charnum] = buffer;  charnum++;   fscanf(f, "%c", &buffer);  }
      charnum = 0;
      break;
    }
    else {
      charnum = 0;
      while (buffer != '\n')  fscanf(f, "%c", &buffer);
    }
  }
}
  fclose(f);
}

I seem to have a mismatch of braces. Though I did a little reformatting I do not think that I messed with the number of braces. I have also, in the interests of space, altered the format of the author's code. Francis

Ok all done with their code, did you find the obvious top four mistakes? Well just in case, here are my answers:

There are a number of other problems here too, such as coping with extra whitespace, the break to exit to while loop, and the fact that this code re-reads the file for each new bot added. There are also a number of stylistic issues too.

Now given that the game code base that this is taken from is entirely C++ I feel that this problem should really have been tackled using C++, besides the code you would produce is neater, simpler and clearer, just the way I like code to be.

#include <fstream>
#include <iostream>
#include <string>
#include <limits>
const int max_bots = 10;

struct BotRecord {
  std::string bot;
  std::string name;
  std::string model;
  int top_colour;
  int bottom_colour;
};
BotRecord g_bot_records[max_bots]; 
int g_bot_count = 0;
bool ReadBotFile(const char* filename) {
  std::fstream in;
  // open the data file
  in.open(filename, std::ios::in);
  if (!in.is_open())   return false;
  // now set up the input stream
  std::istream in_stream(in);
  // read to end of file
  while(!in_stream.eof()) {
    // if its a comment
    if ((in_stream.peek() == '#') || (in_stream.peek() == '\n')) {
      // ignore to the next newline
      in_stream.ignore(std::numeric_limits<int>::max(), '\n');
    }
    else   {             // ok should be a bot config now
      in_stream >> g_bot_records[g_bot_count].bot;
      in_stream >> g_bot_records[g_bot_count].name;
      in_stream >> g_bot_records[g_bot_count].model;
      in_stream >> g_bot_records[g_bot_count].top_colour;
      in_stream >> g_bot_records[g_bot_count].bottom_colour;
      // now ignore the rest of the line
      in_stream.ignore(std::numeric_limits<int>::max(), '\n');
      // increment the bot count
      g_bot_count++;
    }
  }
  return true;
}

The above code is meant to be a simplified example, and I'd suggest some improvements for any production code. I see no reason to use arrays normally and only use one above as I know the size is fixed, however I'd prefer a vector in almost all cases. So if you are going to use C++, make a little effort to learn it, you will save in the long run.

True, but I am not impressed by the original coders C. I am also unimpressed by monolithic functions. Anyone want to redesign this and rewrite the code? I think it might be worth the effort.