Parsing key-value pairs to fill a struct

Question

I must read and extract some values from string. These values are coded like this:

k="11,3,1" v="140.3" 

I have defined the codes and created struct with all field as well as a temp one where I store k and v. In fillFields proc I transfer values from temp struct to the right one (with the valid types). It works but I have many fields and fillFields would need to have many if-conditions. Maybe someone could give me any hint how to write it smarter.

The simplified code now:

   #define ASK                       "11,3,1"
   #define BID                       "11,2,1"
   #define CLOSE                     "3,1,1"


    typedef struct tic {

      float ask;
      float bid;
      float close;
    }tic, *ticP;
    typedef struct pElem {
      char       * k;
      char       * v;
    }pElem, *pElemP;

    void fillFields(ticP t, pElemP p)
    {
      if (strcmp( ASK, p->k)==0)
      {
        printf ("ASK %s\n", p->v);
        t->ask = atof(p->v);
      }
      if (strcmp( BID, p->k)==0)
      {
        printf ("BID %s\n", p->v);
        t->bid = atof(p->v);
      }
      if (strcmp( CLOSE, p->k)==0)
      {
        printf("CLOSE >>>%s<<<\n", p->v)    ;
        t->close =  atof (p->v);
      }
    }

Answer 1

I like to go data driven, so I can write a single loop for processing. First, let's factor out the three if blocks that are so similar into a helper function:

void fillField(pElemP p, char* match, char* message, float* pf)
{
    if (strcmp(match, p->k)==0)
    {
        printf(message, p->v);
        *pf = atof(p->v);
    }
}
void fillFields(ticP t, pElemP p)
{
    fillField(p, ASK, "ASK %s\n", &t->ask);
    fillField(p, BID, "BID %s\n", &t->bid);
    fillField(p, CLOSE, "CLOSE >>>%s<<<\n", &t->close);
}

Now that we have this split out, we're repeating a lot less code, and have identified what information we'd need in order to make this data driven. I'm not sure going pure data driven is much better in this case, but let's look at one way it might be done:

struct FieldSpec {
    char *match;
    char *message;
    int offset;
};

struct FieldSpec field_specs[] = {
    { ASK, "ASK %s\n", offsetof(tic, ask) },
    { BID, "BID %s\n", offsetof(tic, bid) },
    { CLOSE, "CLOSE >>>%s<<<\n", offsetof(tic, close) },
};

void fillFields(ticP t, pElemP p)
{
    for (int i = 0; i < sizeof(field_specs) / sizeof(*field_specs); ++i)
    {
        fillFieldBySpec(field_specs[i], p, t);
    }
}

I'll leave the implementation of fillFieldBySpec up to you if you want to go this route (feel free to put it inside the for loop instead of its own function). I don't think it helps significantly over the first refactoring, and the gynmastics to use the value from offsetof aren't pretty.

Answer 2

How about creating a look-up table for your string literals like such:

#define NUM_COMMANDS (3)
#define ASK          ("11,3,1")
#define BID          ("11,2,1")
#define CLOSE        ("3,1,1")

char *lookUpTable[NUM_COMMANDS] = {ASK, BID, CLOSE};

typedef enum
{
    ASK_CMD = 0,
    BID_CMD,
    CLOSE_CMD,
    END_CMD
} enumCommands;

With this you can use a loop in your fillFields function like such:

void fillFields(ticP t, pElemP p)
{
    for(enumCommands cmd = ASK_CMD; cmd < END_CMD; cmd++)
    {
        if(strcmp(lookUpTable[cmd], p->v) == 0)
        {
            switch(cmd)
            {
                case ASK_CMD:
                    t->ask = atof(p->v);
                    break;
                case BID_CMD:
                    t->bid = atof(p->v);
                    break;
                case CLOSE_CMD:
                    t->close = atof(p->v);
                    break;
            }
        }
    }
}

Answer 3

It seems to me that you are fine tuning the wrong bit. You have already done some parsing to get your pElem structure. If your input string really is as constrained as you say, then maybe you should look at improving the string parsing level. You could do, for example:

#define ASK   "k=\"11,3,1\" v=\""
#define BID   "k=\"11,2,1\" v=\""
#define CLOSE "k=\"3,1,1\" v=\""

if (!strcmp(s, ASK)) {
    ask = atof(s[sizeof(ASK) - 1]);
}
else if (!strcmp(s, BID)) {
    bid = atof(s[sizeof(BID) - 1]);
}
else if (!strcmp(s, CLOSE)) {
    close = atof(s[sizeof(CLOSE) - 1]);
}
else {
    // error handling
}

On your naming, it is usually much clearer if your types are not pointers. For example, your types would be better as:

typedef struct tic {
    float ask;
    float bid;
    float close;
} Tic;

typedef struct elem {
    char *k;
    char *v;
} Elem;

Or miss out the typedef altogether and just use struct tic etc. But as I said above, your element type seems redundant.

Answer 4

I am not sure, what you mean by 'smarter', but here is a possible speed improvement:

If k is always ASK, BID or CLOSE, and nothing else, you can avoid calling strcmp(..) because looking at the 4th character shows which case it is

#define ASK                       '3'  
#define BID                       '2'
#define CLOSE                     ','

typedef struct tic {
  float ask;
  float bid;
  float close;
}tic, *ticP;

typedef struct pElem {
  char       * k;
  char       * v;
}pElem, *pElemP;

void fillFields(ticP t, pElemP p)
{
  switch((p->k)[3]))
  {
    case ASK:
      printf ("ASK %s\n", p->v);
      t->ask = atof(p->v);
      break;
    case BID:
      printf ("BID %s\n", p->v);
      t->bid = atof(p->v);
      break;
    case CLOSE:
      printf("CLOSE >>>%s<<<\n", p->v)    ;
      t->close =  atof (p->v);
  }
}