Nonono. Don't use str_dup. If it goes without being freed you get a memory leak. Use sprintf or strcpy (snprintf or strncpy for safety if you want)

That's still causing a memory leak. Initialise newDesc to NULL, then check if it's NULL, and if not free it before you assign it. Or better yet have a static array and return the address of that, as Davion suggested.

Speaking of memory leaks, there appears to be a condition under which a buffer overflow, crash, or othersuch can occur:

while( *desc != ':' )
      {
         if( *desc == '\0' )
         {
            log_printf( "Error: tag has no parser ':'. Room: %d.", m_InRoom->vnum );
            *common = *desc;
            break;
         }
         *common = *desc;
         *++common = '\0';
         ++desc;
      }

      /* Skip the colon and space ': '. */
      desc += 2;

If *desc comes up as a NUL char, the function shouldn't just keep on running like nothing is wrong. Incrementing the pointer like that and then continuing is likely to have it point to garbage data or worse, should the value of desc be beyond its allocated memory span. :stare:

One thing to think about is that since you're in C++, you can use C++ strings and string methods. (It was how I was going to implement it.) Another thing is to work on what ghasatta was getting at, namely breaking it up into several pieces instead of having one big function.

The core of what you're doing here involves two tasks:

- copying text, until you find a tag
- processing tags

The second can be broken up into more subtasks, such as differentiating the condition from the output if the condition is true, and finally making a way to evaluate the condition. All of this stuff is quite different from the task of just finding the tags.

Anyhow, I really would suggest using C++ strings in this instance, it'll make life a lot easier. (No need to worry about returning with a static buffer or with an allocated string, for example.)

I'm more than willing to use std::string for this, I just have no idea how. :P

I'm not sure I follow you here, looks to me like he's putting the static parts and the variable parts all in one place, and there's no accessibility.


Well, me too, and in my experience this:

# Description:
# [#season summer: You are standing within the expanse of the famous Darkhaven Square.  A
# stone statue of occupies the square's center, surrounded by gardens of
# flowers in full bloom which enhance the air of serenity and peace here in
# the center of the city.  The main road lead away in the cardinal directions, while
# to the northeast and northwest are forested paths.  The spires of a
# cathedral can be seen rising to the northwest.]
# [#season spring: You are standing within the expanse of the famous Darkhaven Square.  A
# stone statue of occupies the square's center, surrounded by gardens of
# budding flowers which fill the air with lovely fragrances here in the center
# of the city.  The main road lead away in the cardinal directions, while
# to the northeast and northwest are forested paths.  The spires of a
# cathedral can be seen rising to the northwest.]
# [#season fall: You are standing within the expanse of the famous Darkhaven Square.  A
# stone statue of occupies the square's center, surrounded by gardens of
# wilting flowers as winter fast approaches here in the center of the city. 
# The main road lead away in the cardinal directions, while to the northeast
# and northwest are forested paths of many color falling leaves.  The spires of a
# cathedral can be seen rising to the northwest.]
# [#season winter: You are standing within the frigid expanse of the famous Darkhaven
# Square.  A stone statue of occupies the square's center, surrounded by an army
# of snowmen the children of the city have built here in the center of the city.
# The main roads have been cleared away in the cardinal directions, while
# to the northeast and northwest are forested paths which are still lightly covered
# in snow.  The spires of a cathedral can be seen rising to the northwest.]

will be a maintenance nightmare.

Well yeah it was a poor example, as he's repeating the same text over and over. In reality the description would look more like this:

You are [#position] within the [#season winter: frigid] expanse of the famous Darkhaven 
Square.  A stone statue of something occupies the square's center, surrounded by 

[#season summer: gardens of flowers in full bloom which enhance the air of serenity and peace]
[#season spring: gardens of budding flowers which fill the air with lovely fragrances]
[#season fall: gardens of wilting flowers as winter fast approaches]
[#season winter: an army of snowmen the children of the city have built]

here in the center of the city.  The main road lead away in the cardinal directions, while to 
the northeast and northwest are forested paths

[#season fall: of many color falling leaves]
[#season winter: which are still lightly covered in snow]

.  The spires of a cathedral can be seen rising to the northwest.

You're trying to do too many tasks with one method (search, test AND replace in the same method). Break it down, dude, before it breaks your head.

Firstly, work out how many different types of tag you have.

To me it seems you have 2 types of tag:

1. variable insertion (#position)
2. variable test and insertion [#season == winter ? frigid : ""]

.. but you cannot (as far as i know) in c reflect a variables name to test the variables value with it (unless you store the var in a hash).

So you should define this "tag" as an object (excuse the crappy c syntax):

..InsertionTag.h..
class InsertionTag {
  char *varName; // position
  InsertionTag(char *name);
  replaceInStr(char *string, char *varValue);

 };

.. InsertionTag.c ..
InsertionTag::InsertionTag(char *name){
       varName = name;
}

void InsertionTag::replaceInStr(char *string, char *varValue){
       //varValue would be the actual position, such as standing or sitting.
       //string is the entire room text
       replace(string, varName, varValue); //using th function from 
                                                       //http://bytes.com/groups/c/223500-how-rep...
}

then you need another tag (conditional) that extends that one:

class ConditionalTag::InsertionTag{
  char *conditionMatch; // value of the variable to match, eg winter, summer, whatever
  char *replacement; // value of the replacement text, eg "leaves are falling down on the floor"

  ConditionalTag(char *name, char *conditionMatch, char *replacement);
  void replaceInStr(char *string, char *varValue);
};


.. ConditionalTag.h..
ConditionalTag::ConditionalTag(char * name, char *match, char *replace){
    name = strcpy(name);
    conditionMatch = strcpy(match);
    replacement= strcpy(replace);
}
void ConditionalTag::replaceInStr(char *string, char *varValue){
       //find in string, replace occurance with replacement
       InsertionTag parent = ((InsertionTag)*this);
       if(varValue == conditionMatch){
         // not sure this is right, basically want to call parent classes functionality. you could just replicate the call tho
          parent->replaceInStr(string, strcat(varName, strcat(strcat(":", name), replacement), replacement); 
       }else{
           // remove the text and tag, we are not wanted
          parent->replaceInStr(string, strcat(varName, strcat(strcat(":", name), replacement), ""); 
       }

}

This way you can create a parser class that goes through the string creating the tag objects on the fly.
/Then/ re-parse your string with each of the created tags and passing the appropriate variable in for the condition. I could write all the code there too, but I hope thats fairly self explanatory…

The only problem is you are iterating over the string a number of times because you're doing it for each tag. Its not too bad but it might be a little wasteful.. perhaps you can modify the string replace function to do some trickery.

if you need more help I'd be welcome to give pointers. no, not those kind of pointers ;)

I really, really would not approach this problem with objects. Decomposition, yes, but objects, no. Not unless you're parsing to a syntax tree or something like that. And definitely not if each tag is going to linearly search/replace in the whole string.

The algorithm can be implemented in a single, simple pass:

- Scanning left to right:
- If the current character isn't a tag begin, copy it to the final string.
Move to next character.

- Else, if it is a tag begin, find the end of the tag. (Encountering an end-of-string is an error while in tag parsing state.)
Pass the tag through the tag handler, and copy the result to the final string.

The tag handler would be responsible for figuring out if it's a conditional tag, finding the condition to test, actually evaluating it, etc.

why not? its the way that jsps do it..

you could pre-define your tags so that the objects can be reused instead of being created all the time.. (flyweight pattern) – and there must be a hundred other pointer tricks you can do to stop the iteration over the whole string..?

with a top down approach its just a little more tricky to keep track of whats happening. not to use objects is plain silly, imho but thats me :D

There's nothing to "keep track of", that's what I'm getting at. It's a simple transformation. Objects are appropriate when you have state to worry about; in this case, there is no state. Objects are also useful when you need a common interface to different implementations; in this case, that hasn't been achieved because the code using the object has to know about what kind of object it is in order to pass in the appropriate "varValue".

FWIW, the string handling in the code you gave is pretty off. :thinking:

What if there is both a winter and a night description?

Would also make more sense to build the option as a var val var val switch: [#season: winter "It's cold." summer "It's warm."]

That's the exact approach I use, and it works fine (although I've been considering adding a two-pass solution as well).

yea my c is pretty ropey, apologees again. thats what you get for being a Java programmer for so many years :(

however, your oo must be pretty cracked too (no offense) because if you think your iterative approach doesn't store state you are sorely mistaken. Apart from that glaring omission, objects are not only good for storing state, I can have a perfectly good object with absolutely no state whatsoever and it can be perfectly serviceable.

What object oriented analysis does do is make you break down the task into chunks and encapsulates functionality (and encourages code re-use, but thats another discussion), and he professed to not understand how to tackle the issue. I was trying to get him to think about it differently - not a bad thing I hope?

Where exactly am I storing state other than during the parsing itself? The tags have no state storage. Even the parsing has no state beyond the life of the function. That is my point: all the required state (of which there is very, very little) is already stored with local variables.


There are many ways to break things into smaller logical chunks. Keep in mind that decomposition does not equate to object orientation. There are times when object orientation is simply not appropriate; this problem as we have seen it so far is one of them.

Give me just one single advantage that OOP has that cannot be achieved otherwise.

you challenge me to provide the impossible (are you flamebating me?) you can always do any programming task multiple ways..

however, when I have a perfectly good pair of scissors, why should I use a knife? I'm better at using my scissors, thanks.

you might be better at using your knife, good for you…

When the only tool you have is a hammer, every problem looks like a nail. Java programmers use objects for freaking everything because Java doesn't give them any more reasonable choices, as Java is a one-trick pony.



No, not really. Not unless you store the parse information across repeat calls. In which case you could compile the whole string into an array of a simple struct like:

struct parse_part_t {
  int type; /* literal, variable, condition */
  const char* string; /* literal value or variable name */
  int op; /* operator */
  const char* compare; /* value for comparisons */
};

For parsing, it would be far easiest to tokenize the tag data. Your parser basically looks for the tag opener with strstr(), spits out everything before the match (if any), then uses the tokenizer to grab the tag internals. When it gets the END token, it evaluates the tag, sets the current input to the end of the tag, and continues on the loop. When the loop ends, spit out whatever is left over. Writing a tokenizer is VERY easy.

Then you just need a very simple parser that works on the tokens, which is far easier to work with conceptually than a big jumbled mess of string operators.

// very C-like pseudo code
do {
  // find beginning of tag
  delim = strstr(input, "[#");

  // append output from before tag
  if (delim != input)
    append_to_output(&output, input, delim - input);

  // expect a name token
  get_token(&input, &token);
  if (token.type != TNAME) {
    // display error however you want
    return;
  }
  variable_name = token.string;

  // check to see if we have an operator
  get_token(&input, &token);
  if (token.type == TOP) {
    operator = token.operator;

    // get a comparison value
    get_token(&input, &token);
    if (token.type != TNAME && token.type != TNUMBER) {
      // parse error
      return;
    }
    comparison_value = token.string;

    // we now expact a : token
    get_token(&input, &token);
    if (token.type != TCOLON) {
      // parse error
      return;
    }

    // find the end of the data, which is at the first ]
    end_marker = strchr(input, ']');
    if (end_marker == NULL) {
      // parse error
      return;
    }

    // do the comparison; if true, we print the data following the : until the first ], otherwise we ignore it
    if (do_comparison(variable_name, op, comparison_value) == TRUE) {
      add_output(input, end_marker - input);
    }

    // set the input to point to just after the ]
    input = end_marker + 1;

   // no operator, so we expect a closing bracket ]
  } else if (token.type == TEND) {
    // evaluate the variable into the output
    eval_variable_to_output(&output, variable_name);
  
  // not an operator and not a closing bracket… parse error
  } else {
    // report parse error
    return;
  }
} while (delim != NULL);

// append remaining input
append_to_output(&output, input, strlen(input));

The tokenizer itself is pretty darn easy. In more pseudo-ish pseudo-code:

get_token {
  skip_whitespace;
  if (*input == 0)
    return token TEOF;

  if (isdigit(*input)) {
    copy_digits;
    return token TNUMBER;
  } else if (isalpha(*input)) {
    copy_alpha;
    return token TNAME;
  } else if (*input == ':') {
    return token TCOLON;
  } else if (*input == ']') {
    return token TEND;
  } else {
    return token TUNKNOWN;
  }
}

You can skip on the tokenizer if you really want, but it does just make the code way cleaner and easier to read than having all that string parsing right into the main block. Especially with the whitespace skipping and such.

if you put a little more time into it you can make the parser way cleaner looking, too. I usually prefer having an accept() and expect() set of functions that does most of the get_token(), token.type comparison, and error handling, but those work best if you're using a language with exception support.

I may be slightly biased on how easier tokenizers and parsers are given that I've written more of them than I can shake a stick at… really shouldn't be much of a problem.

*considers suggesting a boost library again, but then gets distracted by an albino black sheep flash video, and doesn't say anything*

I'm not flamebaiting you. My claim was/is that objects are unnecessary in this case, and only add complexity. The advantage that you have given for OOP so far is encapsulation/decomposition of the problem. I replied (and you did not disagree) that decomposing into functions works. So I am now asking you for the reasons why OOP is superior, since you seem to believe it is superior and told me that my OOP abilities are "cracked" because I disagree.

Obviously you can solve any problem multiple ways. Hopefully just as obviously, many of those ways will be inferior. :wink:


Well, this at least is something we can agree on. My response is that your preference for scissors because you don't know how to use knives as well shouldn't mean that everybody else should learn with that same disparity. To continue with the utensil argument I have programmed with scissors, knives, forks and spoons, and IMO based on that experience your scissor solution is inferior. Maybe it's the one that's best for you, but that is an entirely separate question.

Okay, the utensil analogy is getting stretched further than a sorority girl. Maybe it's time to drop it.