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

kalosz7347

2016-11-30T15:03:07+01:00

Rejestracja: dni
Ostatnio: dni
Postów: 15

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

Kopiuj

#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

Kopiuj

#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:

Kopiuj

#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

Kopiuj

#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

Kopiuj

#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:

Kopiuj

#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:

Kopiuj

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

using namespace std;

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

fasadin

2016-11-30T15:05:04+01:00

Rejestracja: dni
Ostatnio: dni
Postów: 4883

jedno zdanie na temat tego kodu:

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

kalosz7347

2016-11-30T15:14:48+01:00

Rejestracja: dni
Ostatnio: dni
Postów: 15

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 ?

fasadin

2016-11-30T16:04:54+01:00

Rejestracja: dni
Ostatnio: dni
Postów: 4883

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.

Sopelek

2016-11-30T20:14:56+01:00

Rejestracja: dni
Ostatnio: dni
Lokalizacja: Kraków
Postów: 467

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

using namespace w plikach nagłówkowych to bardzo zła praktyka

Kopiuj

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.

Kopiuj

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.

Kopiuj

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.

Kopiuj

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.

Kopiuj

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.

Kopiuj

 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

Liczba odpowiedzi na stronę

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.

Utwórz konto