Kółko i krzyżyk - kod do oceny

0

Prosiłbym o ocenę tego kodu, co zrobiłem nie tak itp. Czy to dobrze, że sporo metod mam publicznych ? Metody są publiczne ze względu na to, że używam ich w metodach innych klas.

Plik Gra.h

#ifndef GRA_H
#define GRA_H
#include "Gracz.h"
#include "Plansza.h"
#include <cstdlib>
using namespace std;

class Gra
{
    public:
        Gra();
        ~Gra();
        void start();

    private:
        Gracz gracz_Pierwszy;
        Gracz gracz_Drugi;
        Plansza plansza;
        void runda();
        void wykonajRuch(int x, int y, int ruch);
};

#endif // GRA_H

Plik gra.cpp

#include "Gra.h"


Gra::Gra()
{

}

Gra::~Gra()
{

}

void Gra::runda()
{
    int x,y,ruch = 1;
    plansza.wyczyscPlansze();

    while(plansza.ktoWygral() == ' ')
    {
        system("cls");
        if(ruch == 1)
        {
            cout << "Ruch (o): " << endl;
        }
        if(ruch == 2)
        {
            cout << "Ruch (x): " << endl;
        }
        plansza.rysujPlansze();

        cout << "Podaj numer kolumny: ";
        cin >> x;
        cout << "Podaj numer wiersza: ";
        cin >> y;

        while(plansza.sprawdz(x,y) != true)
        {
            cout << "Niepoprawne wspolrzedne, podaj je jeszcze raz" << "\n\n";
            cout << "Podaj numer kolumny: ";
            cin >> x;
            cout << "Podaj numer wiersza: ";
            cin >> y;
        }

        wykonajRuch(x,y,ruch);

        if(ruch == 1)
        {
            ruch = 2;
        }
        else
        {
            ruch = 1;
        }
    }

    system("cls");
    plansza.rysujPlansze();

    if(plansza.ktoWygral() == 'o')
    {
        cout << "Wygral gracz o imieniu: " << gracz_Pierwszy.pobierzImie() << endl;
    }
    if(plansza.ktoWygral() == 'x')
    {
        cout << "Wygral gracz o imieniu: " << gracz_Drugi.pobierzImie() << endl;
    }
    if(plansza.ktoWygral() == 'r')
    {
        cout << "Mamy remis" << endl;
    }
}

void Gra::start()
{
    string imie_Pierwsze, imie_Drugie, odp = "tak";
    cout << "Podaj imie pierwszego gracza (o): ";
    cin >> imie_Pierwsze;
    cout << "Podaj imie drugiego gracza (x): ";
    cin >> imie_Drugie;

    gracz_Pierwszy = Gracz(imie_Pierwsze, 'o');
    gracz_Drugi = Gracz(imie_Drugie, 'x');

    while(odp == "tak")
    {
        runda();
        cout << "Czy chcesz zagrac jeszcze raz ? (odpowiedz tak lub nie)" << endl;
        cout << "Odpowiedz: ";
        cin >> odp;
    }

    cout << "\nDziekujemy za gre" << endl;

}

void Gra::wykonajRuch(int x, int y, int ruch)
{
    if(ruch == 1)
    {
        plansza.wstawZnak(x,y,'o');
    }
    if(ruch == 2)
    {
        plansza.wstawZnak(x,y,'x');
    }
}

Plik Gracz.h:

#include <iostream>
#include <windows.h>
#ifndef GRACZ_H
#define GRACZ_H
using namespace std;

class Gracz
{
    public:
        Gracz();
        Gracz(string imie, char znak);
        ~Gracz();
        string pobierzImie();

    protected:

    private:
        char znak;
        string imie;
};

#endif // GRACZ_H

Plik Gracz.cpp

#include "Gracz.h"

Gracz::Gracz()
{

}
Gracz::Gracz(string imie, char znak)
{
    this->imie = imie;
    this->znak = znak;
}

Gracz::~Gracz()
{

}

string Gracz::pobierzImie()
{
    return imie;
}

Plik plansza.h

#include <iostream>
#ifndef PLANSZA_H
#define PLANSZA_H
using namespace std;

class Plansza
{
    public:
        Plansza();
        ~Plansza();
        void wyczyscPlansze();
        void rysujPlansze();
        char ktoWygral();
        void wstawZnak(int x, int y, char znak);
        bool sprawdz(int x, int y);


    private:
        char aktualnaPlansza[3][3];

};

#endif // PLANSZA_H

Plik plansza.cpp:

#include "Plansza.h"

Plansza::Plansza()
{

}

Plansza::~Plansza()
{

}

void Plansza::rysujPlansze()
{
        string przerywnik = "   -------------";
        cout << endl << przerywnik << endl;
        for (int i = 0; i < 3; i++)
        {
            cout	 << " " << i + 1 << " | "
			<< aktualnaPlansza[0][i] << " | "
			<< aktualnaPlansza[1][i] << " | "
			<< aktualnaPlansza[2][i] << " |";
            cout << endl << przerywnik << endl;
        }
        cout << "     1 | 2 | 3 " << endl << endl;
}

void Plansza::wyczyscPlansze()
{
    for(int i = 0; i < 3; i++)
        {
            for(int j = 0; j < 3; j++)
            {
                aktualnaPlansza[i][j] = ' ';
            }
        }
}

char Plansza::ktoWygral()
{
    if((aktualnaPlansza[0][0] == 'o' && aktualnaPlansza[0][1] == 'o' && aktualnaPlansza[0][2] == 'o') ||
        (aktualnaPlansza[1][0] == 'o' && aktualnaPlansza[1][1] == 'o' && aktualnaPlansza[1][2] == 'o') ||
        (aktualnaPlansza[2][0] == 'o' && aktualnaPlansza[2][1] == 'o' && aktualnaPlansza[2][2] == 'o') ||
        (aktualnaPlansza[0][0] == 'o' && aktualnaPlansza[1][0] == 'o' && aktualnaPlansza[2][0] == 'o') ||
        (aktualnaPlansza[0][1] == 'o' && aktualnaPlansza[1][1] == 'o' && aktualnaPlansza[2][1] == 'o') ||
        (aktualnaPlansza[0][2] == 'o' && aktualnaPlansza[1][2] == 'o' && aktualnaPlansza[2][2] == 'o') ||
        (aktualnaPlansza[0][0] == 'o' && aktualnaPlansza[1][1] == 'o' && aktualnaPlansza[2][2] == 'o') ||
        (aktualnaPlansza[0][2] == 'o' && aktualnaPlansza[1][1] == 'o' && aktualnaPlansza[2][0] == 'o'))
    {
        return 'o';
    }
    else if((aktualnaPlansza[0][0] == 'x' && aktualnaPlansza[0][1] =='x' && aktualnaPlansza[0][2] == 'x') ||
        (aktualnaPlansza[1][0] == 'x' && aktualnaPlansza[1][1] == 'x' && aktualnaPlansza[1][2] == 'x') ||
        (aktualnaPlansza[2][0] == 'x' && aktualnaPlansza[2][1] == 'x' && aktualnaPlansza[2][2] =='x') ||
        (aktualnaPlansza[0][0] == 'x' && aktualnaPlansza[1][0] == 'x' && aktualnaPlansza[2][0] == 'x') ||
        (aktualnaPlansza[0][1] == 'x' && aktualnaPlansza[1][1] == 'x' && aktualnaPlansza[2][1] =='x') ||
        (aktualnaPlansza[0][2] == 'x' && aktualnaPlansza[1][2] == 'x' && aktualnaPlansza[2][2] == 'x') ||
        (aktualnaPlansza[0][0] == 'x' && aktualnaPlansza[1][1] == 'x' && aktualnaPlansza[2][2] == 'x') ||
        (aktualnaPlansza[0][2] == 'x' && aktualnaPlansza[1][1] == 'x' && aktualnaPlansza[2][0] == 'x'))
    {
        return 'x';
    }
    else if(aktualnaPlansza[0][0] != ' ' && aktualnaPlansza[0][1] != ' ' && aktualnaPlansza [0][2] != ' '
            && aktualnaPlansza[1][0] != ' ' && aktualnaPlansza[1][1] != ' ' && aktualnaPlansza[1][2] != ' '
            && aktualnaPlansza[2][0] != ' ' && aktualnaPlansza[2][1] != ' ' && aktualnaPlansza[2][2] != ' ')
    {
        return 'r';
    }
    else
    {
        return ' ';
    }

}

void Plansza::wstawZnak(int x, int y, char znak)
{
    aktualnaPlansza[x - 1][y - 1] = znak;
}

bool Plansza::sprawdz(int x, int y)
{
    if(aktualnaPlansza[x - 1][y - 1] != ' ')
    {
        return false;
    }
    return true;
}

Plik main.cpp:

#include <iostream>
#include <cstdlib>
#include "Plansza.h"
#include "Gra.h"
#include "Gracz.h"

using namespace std;

int main()
{
  Gra gra;
  gra.start();
}
1

jedno zdanie na temat tego kodu:

Rozszesz swoj kod zeby plansza byla 10x10, albo 100x100 pamietajac o zasadzie DRY

0

Na razie to nie mam pojęcia jak to zrobić. Właśnie plansze zrobiłem [3][3] aby jedna kratka na jedną literę była. A poza tym kod jest w miarę ok ?

1

skoro napisalem tylko jedno zdanie na temat tego kodu to uwierz mi, ze nie jest ok.

Najpierw naucz sie pisac algorytmy ktore dzialaja generycznie. Wtedy bedziemy sie przyczepiac do kodu.

0

Nie no nie przesadzaj, aż tak źle nie jest...

  1. using namespace w plikach nagłówkowych to bardzo zła praktyka
Gracz gracz_Pierwszy;
        Gracz gracz_Drugi;

imie_Pierwsze, imie_Drugie
niekonsekwentne nazewnictwo w tym miejscu, lepiej pierwszyGracz, drugiGracz, ...
2. Jeśli konstruktory, destruktory, operatory przypisania implementujesz tak jakby to zostało zrobione domyślnie to albo tego nie rób, albo lepiej, = default http://en.cppreference.com/w/cpp/language/default_constructor
3. ruch = 1; to powinien być enum

`while(plansza.ktoWygral() == ' ')` to też

analogicznie wszystkie wystąpienia...
4.

if(ruch == 1)
        {
            cout << "Ruch (o): " << endl;
        }
        if(ruch == 2)
        {
            cout << "Ruch (x): " << endl;
        }

zmienia się jedynie string jaki wyświetlasz. Lepiej zapisać go do zmiennej w zależności od ruch i wypisac
std::string ruchString = ruch == 1 ? "asd" : "asdasd";
std::cout << ruchString << '\n';
5. przeważnie nie potrzebujesz dodatkowej funkcjonalności std::endl, pisz '\n'
6.1.

while(odp == "tak")
    {
        runda();
        cout << "Czy chcesz zagrac jeszcze raz ? (odpowiedz tak lub nie)" << endl;
        cout << "Odpowiedz: ";
        cin >> odp;
    }

lepiej gdyby była to pętla do...while
6.2 zmienne deklaruj albo najbliżej użycia, albo w miejscu gdzie była deklarowana zmienna o podobnym przeznaczeniu. zmiennej odp chwile szukałem.
7.

Gracz::Gracz(string imie, char znak)
{
    this->imie = imie;
    this->znak = znak;
}

używaj listy inicjalizacyjnej konstruktora http://en.cppreference.com/w/cpp/language/initializer_list
8.

string Gracz::pobierzImie()
{
    return imie;
}

lepiej gdybyś zwrócił jako const std::string&, wtedy nie robisz kopii niepotrzebnie
9. char Plansza::ktoWygral() do przemyślenia jak to zrobić pętlami
10.

 if(aktualnaPlansza[x - 1][y - 1] != ' ')
    {
        return false;
    }
    return true;

niepotrzebny warunek, zwróć po prostu aktualnaPlansza[x - 1][y - 1] == ' ', w dodatku mało mówiąca nazwa zmiennej
11. Próbuj pisać kod po angielsku

1 użytkowników online, w tym zalogowanych: 0, gości: 1