Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a beginner at C++ and I would like to hear your opinion about my code. I don't think it's necessary to desrcribe what the program really does, because it works fine, although there are some special occasions that can result in a crash. Could you take a look at my code and tell me what I could improve?

    using namespace std;

void explode(char* str, int* output){
    char temp[20];
    int tr = 0;
    int ot = 0;
    for(int x = 0;x<strlen(str);x++){
        if(str[x] != *" "){
                temp[tr++] = str[x];
        }
        else{
            temp[tr] = '\0';
            tr= 0 ;
            sscanf(temp, "%d", &output[ot++]);
        }
    }
            temp[tr] = '\0';
            tr= 0 ;
            sscanf(temp, "%d", &output[ot++]);
}

int main(int argc, char** argv) {
    int n = 0;
    cin >> n;
    int* nulaX[n];
    int* nulaY[n];
    cin.ignore(256, '\n');
    int* r[n];
    int* stepsAmmount = new int[n];
    int* steps[n];
    short* pole[n][300];
    for(int wH = 0;wH<n;wH++){
        r[wH] = new int;
        nulaX[wH] = new int;
        nulaY[wH] = new int;
        cin >> *r[wH];
        cin.ignore(256, '\n');
        int* gg = new int[*r[wH]];


        char* input = new char[*r[wH]];

        for(int x = 0;x<*r[wH];x++){
            cin.getline(input,250);
            pole[wH][x] = new short[*r[wH]];
            explode(input, gg);
            for(int y = 0;y<*r[wH];y++){
                pole[wH][x][y] = gg[y];
                if(pole[wH][x][y] == 0){
                    *nulaX[wH] = x;
                    *nulaY[wH] = y;
                }
            }
        }

        cin >> stepsAmmount[wH];
        cin.ignore(256, '\n');
        steps[wH] = new int[stepsAmmount[wH]];
        char* tempC = new char[1000];
        cin.getline(tempC,250);
        explode(tempC, steps[wH]);

    }

        int step, tempX, tempY;
    for(int wH = 0;wH < n;wH++){
        for(int x = 0;x<stepsAmmount[wH];x++){
            step = steps[wH][x];

            if(*nulaX[wH]-1 >= 0 && pole[wH][*nulaX[wH]-1][*nulaY[wH]] == step){
                tempX = *nulaX[wH]-1;
                tempY = *nulaY[wH];
            }
            else if(*nulaX[wH]+1 < *r[wH] && pole[wH][*nulaX[wH]+1][*nulaY[wH]] == step){
                tempX = *nulaX[wH]+1;
                tempY = *nulaY[wH];
            }

            else if(*nulaY[wH]-1 >= 0 && pole[wH][*nulaX[wH]][*nulaY[wH]-1] == step) {
                tempX = *nulaX[wH];
                tempY = *nulaY[wH]-1; 
            }

            else if(*nulaY[wH]+1 < *r[wH] && pole[wH][*nulaX[wH]][*nulaY[wH]+1] == step){
                tempX = *nulaX[wH];
                tempY = *nulaY[wH]+1;
            }

            else{
                cout << "Pokus o utok" << endl;
                return 0;
            }

            pole[wH][*nulaX[wH]][*nulaY[wH]] = pole[wH][tempX][tempY];
            pole[wH][tempX][tempY] = 0;
            *nulaX[wH] = tempX;
            *nulaY[wH] = tempY;
        }



        int m = 0;
        bool result = true;
        for(int x = 0;x<*r[wH];x++){
            for(int y = 0;y<*r[wH];y++){
                if(pole[wH][x][y] == 0){
                    continue;
                }
                else if(++m != pole[wH][x][y]){
                    x = *r[wH];
                    result = false;
                    break;
                }
            }
        }

        if(result == true){
            cout << "Prihlaseny do systemu" << endl;
        }
        else{
            cout << "Cas vyprsal" << endl;
        }
    }
}
share|improve this question

1 Answer

up vote 5 down vote accepted

I'll go ahead and answer this as a Code Review question, even though StackOverflow would be more correct for specifically finding the crashes. Let's take a look:

    using namespace std;

You shouldn't do this. The std namespace is big, and if you want to use part of it, just do using std::cout; or similar. Even better, explicitly prefix everything so it's clear what variables and functions come from where.

By the way, where are your includes? Surely you're using some parts of the C++ standard library, so you should #include them.

void explode(char* str, int* output){
    char temp[20];
    int tr = 0;
    int ot = 0;

This would be fine in C, but in C++, there's no need for it. Use std::strings, not char pointers, and you might fix your crashing bugs without even realising it. A lot of bugs happen due to code that doesn't check array bounds and such, and writing things like this is just asking for it.

     for (int x = 0; x < strlen(str); x++) {
         if (str[x] != *" ") { // Should be: (str[x] != ' ')
             temp[tr++] = str[x];
         } else {
             temp[tr] = '\0';
             tr = 0;
             sscanf(temp, "%d", &output[ot++]);
         }
     }

Here's the kind of code I mean. Are you sure str is not NULL? You know temp has a size of 20 -- check that you don't write more to it! Passing in a string that's too big isn't going to do you any good.

By the way, notice that I fixed your whitespace up a little. Try to be more consistent in the future; it'll save people reading your code (including you) a lot of headaches.

    temp[tr] = '\0';
    tr= 0 ;
    sscanf(temp, "%d", &output[ot++]);

This shouldn't be indented that far. Also, this is exactly the same as what's in the for loop; why not make it a function?

All in all, this function does almost no checking for null or out-of-bounds cases, and that's a Bad Thing. It may be causing your errors. Add checks that th

int main(int argc, char** argv) {
    int n = 0;
    cin >> n;
    int* nulaX[n];
    int* nulaY[n];
    cin.ignore(256, '\n');
    int* r[n];
    int* stepsAmmount = new int[n];
    int* steps[n];
    short* pole[n][300];

This isn't legal C++ code; it is a GCC extension that lets you make variable-size arrays. Use an std::vector. Also, what in the world do you need so many arrays of int pointers for? I'll also point out immediately that you're leaking the memory taken up by stepsAmmount; you never delete[] anything, but you do use new[]. Does your program crash due to out-of-memory errors?

You're also ignoring only 256 characters after reading that int. It may be enough often, but reading the maximum number is likely to be just as safe (and just as fast in most cases, that is, any cases where the newline is in less than 256 characters).

    for (int wH = 0; wH < n; wH++) {
        r[wH] = new int;
        nulaX[wH] = new int;
        nulaY[wH] = new int;
        cin >> *r[wH];
        cin.ignore(256, '\n');
        int* gg = new int[*r[wH]];

You've created ten arrays now, and they all have fairly meaningless names. I have not yet figured out what your program does. I probably won't any time soon, either.

Why is r an array of int pointers, not an array of ints? Same about nulaX and nulaY, really. What is the user supposed to enter here? Even if you don't want to print it, at least put it in a comment so whoever is reading will know waht to expect.

        char* input = new char[*r[wH]];

Use an std::string!

        for(int x = 0;x<*r[wH];x++){
            cin.getline(input,250);
            pole[wH][x] = new short[*r[wH]];
            explode(input, gg);
            for(int y = 0;y<*r[wH];y++){
                pole[wH][x][y] = gg[y];
                if(pole[wH][x][y] == 0){
                    *nulaX[wH] = x;
                    *nulaY[wH] = y;
                }
            }
        }

Use std::getline(std::cin, my_string);. Why are you dynamically allocating *r[wH] chars and always reading 250? As far as I can tell, so far you've just populated two arrays with coordinates and some kind of pole thing. You could do this much simpler: just make an std::vector<std::pair<int, int> > for the coordinates, and maybe an std::vector<std::vector<std::vector<int> > > for the gg things, whatever they are. The latter may not even be necessary, as it's very unclear what you're actually trying to do.

        cin >> stepsAmmount[wH];
        cin.ignore(256, '\n');
        steps[wH] = new int[stepsAmmount[wH]];
        char* tempC = new char[1000];
        cin.getline(tempC,250);
        explode(tempC, steps[wH]);
    }

Whatever this is, it can be rewritten using the C++ Standard Library to be much neater. Again, why are you allocating 1000 chars and then only reading 250?

I'm starting to guess you're solving a differential equation or something like that. Have you considered Python? It's a lot more intuitive, and SciPy may make it fast enough for the purpose.

    int step, tempX, tempY;
    for(int wH = 0;wH < n;wH++){
        for(int x = 0;x<stepsAmmount[wH];x++){
            step = steps[wH][x];

            if (*nulaX[wH]-1 >= 0 && pole[wH][*nulaX[wH]-1][*nulaY[wH]] == step) {
                tempX = *nulaX[wH]-1;
                tempY = *nulaY[wH];
            } else if (*nulaX[wH]+1 < *r[wH] && pole[wH][*nulaX[wH]+1][*nulaY[wH]] == step){
                tempX = *nulaX[wH]+1;
                tempY = *nulaY[wH];
            } else if (*nulaY[wH]-1 >= 0 && pole[wH][*nulaX[wH]][*nulaY[wH]-1] == step) {
                tempX = *nulaX[wH];
                tempY = *nulaY[wH]-1; 
            } else if(*nulaY[wH]+1 < *r[wH] && pole[wH][*nulaX[wH]][*nulaY[wH]+1] == step) {
                tempX = *nulaX[wH];
                tempY = *nulaY[wH]+1;
            } else {
                cout << "Pokus o utok" << endl;
                return 0;
            }

This could almost certainly be split into functions. Even just an array of pairs (-1, 0), ( 0, -1), (1, 0), (0, 1) would already make it possible to rewrite this in a far more generic manner. In your case, more functions is definitely better -- if you see yourself writing the same thing multiple times, see if you can split it off in any way.

            pole[wH][*nulaX[wH]][*nulaY[wH]] = pole[wH][tempX][tempY];
            pole[wH][tempX][tempY] = 0;
            *nulaX[wH] = tempX;
            *nulaY[wH] = tempY;
        }

This is too cryptic to comment on.

        int m = 0;
        bool result = true;
        for(int x = 0;x<*r[wH];x++){
            for(int y = 0;y<*r[wH];y++){
                if(pole[wH][x][y] == 0){
                    continue;
                }
                else if(++m != pole[wH][x][y]){
                    x = *r[wH];
                    result = false;
                    break;
                }
            }
        }

Again, too cryptic, but the ++m in the condition is almost certain to throw some poor reader of your code off.

        if(result == true){
            cout << "Prihlaseny do systemu" << endl;
        } else {
            cout << "Cas vyprsal" << endl;
        }
    }
}

If you're submitting code for code review, please translate it to English; we'd like to know what your string literals and variable names mean.

In general: use less pointers and more of the standard library, and your bugs will probably heal. I would advise getting a good book on C++, such as Accelerated C++.

share|improve this answer
As soon as i can i will rewrite this code and all variables so you can understand it better. By the way, this program is doing something like this : user inputs a unsolved puzzle (tinyurl.com/6tbz65y) and then he inputs certain steps how to solve this puzzle (order numbers). Program then analyses that puzzle was solved or not. You asked why so much pointers - That's because everything is needed to be allocated dynamically (as it all depends on user's input) and the only way i know is by "new" keyword, which returns pointer. Anyway thank you very much for now – user10099 Feb 2 '12 at 1:49
ok, i added few comments to my code and edited few things, i hope it will help. i can't add it here so here is link : kooo.ssdd.cz/cpp.txt – user10099 Feb 2 '12 at 15:03

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.