Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So the task is that a string being passed to one of my methods looks like this

<DIV><GKY><UID><END>

it is generated this way from another program, so it will always have that format. Now to sum up how it works is like this.. This is to represent 3 byte arrays a DIV key (short for diversification) a Guest Key (GKY), and a Unique ID (UID). This particular class that i made stores those byte arrays as a 2 character hex string. IE {0x03, 0xFE, 0xE0} would be stored as "03FEE0"

so those are the requirements of the class.. I have to be able to parse that string to store those byte arrays for access later. DIV (if present) will always be 16 bytes long. GKY is variable and can be at most 192 bytes. UID is required and will always be present and is always 7 bytes. so I came up with this... as far as I can tell it works.

#pragma once

class KeyInputParser
{
public:
    KeyInputParser(void)
    {
        //DIV will always be 16bytes... but it is either not required
        DIV = new char[32];
        //uid will ALWAYS be 7 bytes and is required
        UID = new char[14];
    }
    ~KeyInputParser(void)
    {
        delete[] DIV;
        delete[] GKY;
        delete[] UID;
    }
    char* DIV;
    char* GKY;
    char* UID;
    void SetKeyString(const char* str)
    {
        wcout << "KeyInputParser::SetString()+" << endl;
        wcout << "\t" << str << endl;

        char* div;
        char* gky;
        char* uid;

        div = (char*)memchr(str, '>', strlen(str));
        div++;

        gky = (char*)memchr(div, '>', strlen(div));
        gky++;

        uid = (char*)memchr(gky, '>', strlen(gky));
        uid++;

        for(int i=0; i<strlen(div); i++)
        {
            if(div[i] == '<')
            {//DIV will always be 16bytes... but it is either not required
                memcpy(DIV, div, i);
                DIV[i] = '\0';
                break;
            }
        }
        for(int i=0; i<strlen(gky); i++)
        {//gky will vary, but max out at 192 bytes
            if(gky[i] == '<')
            {
                GKY = new char[i+1];
                memcpy(GKY, gky, i);
                GKY[i] = '\0';
                break;
            }
        }
        for(int i=0; i<strlen(uid); i++)
        {//uid will ALWAYS be 7 bytes and is required
            if(uid[i] == '<')
            {
                memcpy(UID, uid, i);
                UID[i] = '\0';
                break;
            }
        }

        wcout << "KeyInputParser::SetString()-" << endl;
    }
};

I don't care for the way it came out, but I am very new to C++.

share|improve this question

2 Answers 2

up vote 2 down vote accepted

First of all separate code into header (.hpp) and implementation file (.cpp)

Header : (my_header.hpp)

#ifndef MY_HEADER
#define MY_HEADER
class KeyInputParser
{
public:
    KeyInputParser  () // there is no need for funcName (void) in C++
    ~KeyInputParser () // that is neccessary only for C
    char* DIV;
    char* GKY;
    char* UID;
    void SetKeyString (const char* str);
    // Progress bewteen div, gky and uid
    char* Progress (char* previous);
    // Get data between <>
    void ExtractField (char* small, char* big);
};
#endif

Implementation : (my_code.cpp)

#include <cstring> 
#include "my_header.hpp"

KeyInputParser::KeyInputParser () 
{
    //DIV will always be 16bytes... but it is either not required
    DIV = new char[32];
    //uid will ALWAYS be 7 bytes and is required
    UID = new char[14];
}

KeyInputParser::~KeyInputParser () 
{
    delete[] DIV;
    delete[] GKY;
    delete[] UID;
}

char* KeyInputParser::Progress (char* previous) 
{
    char* arr = (char*) memchr (previous, '>', strlen (previous));
    arr++;
    return arr;
}

void KeyInputParser::ExtractField (char* small, char* big) 
{
    for(int i = 0; i < strlen (small); i++)
    {
        if(small [i] == '<')
        {
            memcpy (big, small, i);
            big[i] = '\0';
            break;
        }
    }
}
void KeyInputParser::SetKeyString (const char* str) 
{
    char* div = Progress (str);
    char* gky = Progress (div);
    char* uid = Progress (gky);

    ExtractField (div, DIV);
    // Code for GKY will be copied due to allocation
    // It might be possible to fix it to be used with ExtractField function
    for(int i = 0; i < strlen (gky); i++)
    {
        if(gky[i] == '<')
        {
            GKY = new char[i+1];
            memcpy(GKY, gky, i);
            GKY[i] = '\0';
            break;
        }
    }
    ExtractField (uid, UID);
}

This is just a rough refactoring of your code. The thing number one that will need to be fixed is to drop all those C string functions and use std::string instead.

I did not test the code so I cannot guarantee that it will work but this is how it should look like. Hope I helped :)

share|improve this answer
1  
Your code seems to work great, and looks a billion times better too. I really wish I had started this project and hadn't inherited it otherwise I would use string. I only had to change 2 things in your code to make it work. one was to add const to the input parameter in Progress. And the other was to rename small to something else. small was predefined word in Visual studio. –  Robert Snyder Feb 15 '13 at 16:25

To add to the previous poster, while some would say stay away from #pragma once and use include guards I say use away IF and only IF you know it will only be compiled on one compiler(assuming its a proprietary piece of software) if it is an open source project make sure to use include guards after you do the the pragma something like this

#pragma once
#ifndef HEADER_H
#define HEADER_H
//class definition
#endif //HEADER_H

At least then in that case pragma will be used first and speed up compilation if its supported, if not it will be ignored and the include guards will be used instead. #pragma once directive tells the compiler to include this file only once whereas include guards check to see if this macro has already been defined previously. Also I would do

std::string DIV;
std::string GKY;
std::string UID;

This would avoid needing to do any news or deletes in the constructor and destructor. std::string is a mutable data structure. Also separate the implementation and the class definition and avoid using too many C style functions if there is a C++ way of doing it. Also this method is much too long,

void SetKeyString(const char* str)
{
    wcout << "KeyInputParser::SetString()+" << endl;
    wcout << "\t" << str << endl;

    char* div;
    char* gky;
    char* uid;

    div = (char*)memchr(str, '>', strlen(str));
    div++;

    gky = (char*)memchr(div, '>', strlen(div));
    gky++;

    uid = (char*)memchr(gky, '>', strlen(gky));
    uid++;

    for(int i=0; i<strlen(div); i++)
    {
        if(div[i] == '<')
        {//DIV will always be 16bytes... but it is either not required
            memcpy(DIV, div, i);
            DIV[i] = '\0';
            break;
        }
    }
    for(int i=0; i<strlen(gky); i++)
    {//gky will vary, but max out at 192 bytes
        if(gky[i] == '<')
        {
            GKY = new char[i+1];
            memcpy(GKY, gky, i);
            GKY[i] = '\0';
            break;
        }
    }
    for(int i=0; i<strlen(uid); i++)
    {//uid will ALWAYS be 7 bytes and is required
        if(uid[i] == '<')
        {
            memcpy(UID, uid, i);
            UID[i] = '\0';
            break;
        }
    }

    wcout << "KeyInputParser::SetString()-" << endl;
}

break it up into smaller methods that pass the work between them, makes it more readable and maintainable. Also change the char *'s in the program to use std::string I'm going to assume you're a newish programmer so you might think "nobody is going to be maintaining my code" while that may be true, you should act like they will be and get into the habit of using best practices. Hope this helps.

share|improve this answer
    
although I am not a new programmer, I am new to C++. I have learned the lesson that others will maintain my code but thank you for the reminder. I have had a few moments when reviewing my own code a few months after the fact that I was like...what was I thinking... which is about the same as someone else editing my code. I tried using string the other day with this and was having a hard time getting it to work properly. I tried only with one variable (UID) since it was always present, but I had problems. I should try again though. –  Robert Snyder Feb 26 '13 at 17:06
    
Glad to be of help, changing those char *'s to std::string's should be easy enough and instead of using strlen() you can just do something like this std::string whatever = "abcdefgh"; whatever.length(); –  David Tr Feb 26 '13 at 18:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.