1
\$\begingroup\$

Since I'm new to coding, can you give me some advise on game class interactions? The game does work, but my code looks ugly and I think it can be improved a lot.

What I'm mostly interested in:

  1. class1.setvalue(class2.returnvalue()) is this how it is supposed to be done?
  2. Should I derive my objects from some virtual base?
  3. If I would like to add another enemy (or food) class object at some point of the game, how should it be done?
  4. The use of int k to make enemy move in only half of cycles.

Don't go rough on console things. No one makes console games anyway.

#include "stdafx.h"
#include <iostream>
#include <conio.h>
#include <vector>
#include <windows.h>
using namespace std; //know its a bad practise

struct coord
{
    int x;
    int y;
    char mark;
};

class karta //map
{
public:
    karta(int a, int b )
    {
        mx = a; my = b;
        for (i = 0; i < mx; i++)
        {
            vector<char> clm; 
            for (j = 0; j < my; j++)
            {
                clm.push_back(' '); 
            }
            mapa.push_back(clm); 
        }
    }
void generatemap()  
    {
        for (i = 1; i < mx; i++)
            for (j = 1; j < my; j++)
                mapa[i][j] = ' ';

        for (i = 1; i <= my; i++)
        {
            mapa[0][i-1] = 177;
            mapa[mx-1][i-1] = 177;
        }
        for (i = 1; i <= mx; i++)
        {
            mapa[i-1][0] = 177;
            mapa[i-1][my-1] = 177;
        }
    }
void generateobjects() 
{
    mapa[food.x][food.y] = food.mark;
    mapa[hero.x][hero.y] = hero.mark;
    mapa[enemyc.x][enemyc.y] = enemyc.mark;
}
void printemap() 
    {
        for (j = 0; j < my; j++)
        {
            for (i = 0; i < mx; i++)
            cout << mapa[i][j];
            cout << endl;
        }
    }

void updateobjectdata(coord k)
{
    hero = k;
}
void setfoodc(coord k) 
{
    food = k;
}
void updatenemy(coord k)
{
    enemyc = k;
}

int pass_x() 
{
    return mx;
}
int pass_y() 
{
    return my;
}

private:
    int i,j;
    int mx;
    int my;
    vector< vector<char> > mapa; 
    coord hero,food,enemyc;
};

class game
{
public:
    game() 
    {
        running = true;
        score = 0;
        flag = true; 
    }

void setwalls(int x, int y)
    {
        wallx = x-1; wally = y-1;
    }
void sethero(coord k)
    {
        heroc = k;
    }
void setfood(coord k)
    {
        foodc = k;
        flag = false;
    }
void setenemy(coord k)
    {
        enemyc = k;
    }

bool ifrunning()
    {
        if (running)
            return true;
    }

void checkwall()
    {
        if (wallx == heroc.x || wally == heroc.y || heroc.x == 0 || heroc.y == 0)
        {
            running = false;
        }

    }
void checkenemy()
    {
        if (heroc.x == enemyc.x && heroc.y == enemyc.y)
        {
            running = false;
        }
    }

int enemymovement()
    {
        if (heroc.x > enemyc.x)
            if (heroc.y > enemyc.y)
                if (rand() % 2 == 1)
                    return 1;
                else
                    return 3;
            else
                if (heroc.y == enemyc.y)
                    return 1;
                else
                    if (rand() % 2 == 1)
                        return 1;
                    else
                        return 4;
        else
            if (heroc.y > enemyc.y)
                if (heroc.x == enemyc.x)
                    return 3;
                else
                    if (rand() % 2 == 1)
                        return 2;
                    else
                        return 3;
            else
                if (heroc.x == enemyc.x)
                    return 4;
                else
                    if (heroc.y == enemyc.y)
                        return 2;
                    else
                        if (rand() % 2 == 1)
                            return 2;
                        else
                            return 4;

    }

void checkfood()
    {
        if (heroc.x == foodc.x && heroc.y == foodc.y)
        {
            score += 5;
            flag = true;
        }
    }

bool flagstatus()
    {
        return flag;
    }

void gameover() 
    {
        cout << "Game over! Your score is :" << score;
    }

private:
    bool running,flag;
    int wallx, wally, score;
    coord heroc,foodc,enemyc;
};

class enemy
{
public:
    enemy(int a, int b)
    {
        limx = a - 2; limy = b - 2;
        obj.mark = 'G';
    }
void gencoord()
    {
        obj.x = rand() % limx + 1;
        obj.y = rand() % limy + 1;
    }
void move(int a)
    {
        switch (a)
        {
        case 1:
            obj.x++;
            break;
        case 2:
            obj.x--;
            break;
        case 3:
            obj.y++;
            break;
        case 4:
            obj.y--;
            break;
        }
    }

coord givecord()
    {
        return obj;
    }
private:
    coord obj;
    int limx, limy;
};

class object //hero
{
public:
    object()
    {
        obj.x = 6;
        obj.y = 6;
        obj.mark = '$';
    }

void move(char key)
    {
        switch (key)
        {
        case 'w':
            obj.y--; // inversed!
            break;
        case 'a':
            obj.x--;
            break;
        case 'd':
            obj.x++;
            break;
        case 's':
            obj.y++; // inversed!
            break;
        }
    }

coord givecord()
{
    return obj;
}

private:
    coord obj;
};

class food
{
public:
    food(int a, int b) 
    {
        limx = a-2; limy = b-2;
        obj.mark = '5';
    }

void gencoord()
    {
        obj.x = rand() % limx +1;
        obj.y = rand() % limy +1;
    }

coord givecord()
    {
        return obj;
    }

private:
    coord obj;
    int limx, limy;
};

int main()
{
    karta mapka(30,15); //size depends on your console
    object hero;
    game gamz;
    food eda(mapka.pass_x(), mapka.pass_y());
    enemy vrag(mapka.pass_x(), mapka.pass_y());
    vrag.gencoord();
    int k = 0;

    gamz.setwalls(mapka.pass_x(), mapka.pass_y());
    gamz.sethero(hero.givecord());
    gamz.setenemy(vrag.givecord());

    mapka.generatemap();
    mapka.updateobjectdata(hero.givecord());
    mapka.updatenemy(vrag.givecord());
    mapka.generateobjects();
    mapka.printemap();

    Sleep(2000);
    _getch();

    while (gamz.ifrunning() == true)
    {
        Sleep(110);
        if (_kbhit()) 
        {
            hero.move(_getch());
        }
        gamz.sethero(hero.givecord());
        gamz.checkenemy();
        gamz.checkwall();
        gamz.checkfood();
        if (gamz.flagstatus() == true)
        {
            eda.gencoord();
            gamz.setfood(eda.givecord());
            mapka.setfoodc(eda.givecord());
        }

        if (k%2 == 1)
            vrag.move(gamz.enemymovement());
        k++;
        gamz.setenemy(vrag.givecord());
        gamz.checkenemy();

        system("cls");
        mapka.updateobjectdata(hero.givecord());
        mapka.updatenemy(vrag.givecord());
        mapka.generatemap();
        mapka.generateobjects();
        mapka.printemap();
    }
    gamz.gameover();

    Sleep(5000);
    _getch();

    return 0;
}

$ - you; 5 - food; G - enemy; WASD - move. Game language must be English

\$\endgroup\$
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Mast Sep 27 '16 at 13:53
  • \$\begingroup\$ Also, what's the last part of your question saying, everything under the code block? \$\endgroup\$ – Mast Sep 27 '16 at 13:54
  • \$\begingroup\$ @Mast That would be the instructions how to play the game. \$\endgroup\$ – Ext3h Sep 27 '16 at 13:55
4
\$\begingroup\$
struct coord
{
    int x;
    int y;
    char mark;
};

OOP is not only about how you encapsulate data and functions into objects - it's also about defining how objects interact with each other.

Let's pick the coord object as an example. As you use it, it's just a container, but not exactly object oriented. This line actually characterizes it well:

heroc.x == foodc.x && heroc.y == foodc.y

Despite having two coord instances which are in a trivial relation to each other, you couldn't just compare them, because you didn't implement cood::operator==(). Instead you needed to copy the corresponding logic into every single site attempting to compare two coord instances.


mapa[0][i-1] = 177;

What is 177? OK, I know from the context it's an extended ASCII code. But without looking it up in a table, I don't know which one.

This makes 177 a magic number.

The problem with that is, you reused it multiple times, and every single time in numeral form. If I wanted to switch out the ASCII symbol used for the border, I would need to check all occurrences of this number. But even if I did - I could not tell whether it had the same semantic.

Use constants instead of magic numbers:

enum class symbol {
    empty = ' ',
    player = '$',
    food = '5',
    enemy = 'G',
    border = 177
}

// Sample usage
map[i][j] = symbol::empty;

Now it's both clear what each symbol means when occurring in the code base, and there is now only a single location which needs to be changed when you want to change the symbols.


if (rand() % 2 == 1)
    return 1;
else
    return 3;

This case of magic numbers is even worse, because this time I can't even deduce from the context what these numbers are supposed to mean.

However, this time the solution is not to uses constants, there is a better solution. Remember how you didn't complete the work on coord?

It's time to implement another operator, coord::operator+(). With that operator implemented, you can simply return an coord instance representing the offset the enemy is supposed to move, and add it to the enemies current location. Absolutely no magic number needed this time.


game gamz;
food eda;
enemy vrag;

Mixing languages is typically a horrible idea. A sane guideline, stick to the language predominant in the framework you are using. In the case of C++ and the STL, that's exclusively English.

This is not only because other developers most likely won't be able to understand why you named something in a specific way - it's also ambiguous. For every possible variable, you want to have a naming scheme which allows only one sensible name for that variable so that you never have to guess how you named something. Having two languages to choose from, makes this already a futile attempt.


generatemap
generateobjects
printemap
setfoodc
...

All of these names are actually rather difficult to read. Why? Because chaining words without gaps doesn't work all to well.

You should make use of a naming convention which clearly separates the individual words forming an identifier. The two usual conventions for that are cammelCase and snake_case, with the former one being commonly used in C# or Java, and the latter one being typical in C and C++.


Sleep(2000);
_getch();

And now we halt the program for 2 seconds, and expect the user to know that he should press a key to continue.

There is the instructions missing. Either that, or that delay is entirely pointless.

Sleep(5000);
_getch();

And because it was so nice, we do it again, but this time we prevent the user from terminating the application.


if (_kbhit()) 
{
    hero.move(_getch());
}

Nice try, but this processes only one character per frame, without discarding any other inputs pending in the input buffer. So if the user smashed the keyboard during the aforementioned sleep in expectation of a reaction, all these unprocessed inputs are now being executed one by one.


_kbhit();
_getch();
system("cls");

All 3 of these only work as expected (or at all!) on Windows. Cross platform input handling and interaction with the terminal is rather difficult.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your review. Yet i dont quite understand 3rd part of your answer. My code there makes enemy move on x OR y axis at random. 1 dot/frame. If enemy would move by offset/frame wouldn't it "catch" player instantly? \$\endgroup\$ – Deflopator Sep 27 '16 at 15:49
  • \$\begingroup\$ @Deflopator Moving by an offset simply means you return one of {0,1}, {0,-1}, {1,0} or {-1,0}. That's the offset the enemy moves. Not the offset between player and enemy. \$\endgroup\$ – Ext3h Sep 27 '16 at 16:00
  • \$\begingroup\$ Ok, i do understand that using coord would be easier. But should i still use Rand()%/2 to determine if i need {0,1} or {1,0}? \$\endgroup\$ – Deflopator Sep 27 '16 at 16:39
  • \$\begingroup\$ @Deflopator Yes, the logic for how you decide that doesn't change. Only the interface of your classes does. \$\endgroup\$ – Ext3h Sep 27 '16 at 17:39
1
\$\begingroup\$

So @Ext3h did already cover a lot. here some more thoughts.

  1. If you know its bad dont do it. --> looking at namespace std;

  2. For gods sake use braces. You are in your 5th level of nesting without a single brace. That is simply a bug in waiting. As a rule of thumb, whenever you nest something use braces. It is not like the additional line is hurting you.

  3. Similar to the above declare each variable on its own line. That way it is much easier to read.

  4. Use separate files for each class you have. That makes it much easier to follow your code.

  5. Your main function is loaded with stuff that should be part of the class initialization.

    gamz.setwalls(mapka.pass_x(), mapka.pass_y());
    gamz.sethero(hero.givecord());
    gamz.setenemy(vrag.givecord());
    
    mapka.generatemap();
    mapka.updateobjectdata(hero.givecord());
    mapka.updatenemy(vrag.givecord());
    mapka.generateobjects();
    mapka.printemap();
    

Try to think what your class need to be fully defined and put that in the constructor. As a rule of thumb, whenever you need to do something outside of a class object before you can safely use it, something is wrong.

\$\endgroup\$
  • \$\begingroup\$ Can you explain "2" a bit more? And by "4" you mean object.h + object.cpp? \$\endgroup\$ – Deflopator Sep 27 '16 at 15:55
  • \$\begingroup\$ The function 'enemymovement()' is nested 5 levels deep with if/else ladders without braces. Any slight typo might break havoc there. With 4 i would separate players, game and enemy into respective .h/cpp files \$\endgroup\$ – miscco Sep 27 '16 at 18:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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