Wątek przeniesiony 2020-03-19 13:45 z C/C++ przez kq.

Sprawdzenie kodu w C++

0

Hej,

Dopiero co zacząłem pisać w C++ po dłuższej przerwie. Kiedyś jak miałem z 15-16 lat coś tam pisałem, ale unikałem z reguły OOP. Dwa dni temu napisałem grę w kółko i krzyżyk w konsoli. Teraz zająłem się grą karcianą w wojnę. Ogólnie nie jestem pewny jednej zasady tej gry, a mianowicie kiedy jest wojna to jak są sprawdzane karty, przyjąłem, że tylko ta trzecia. Więc jak wprowadziłem złe założenie to przepraszam. Jest tu może ktoś chętny doświadczony, kto oceniłby mój kod? Chodzi mi głównie o taką ocenę co robię źle takie coś w rodzaju dobrych praktyk programistycznych. Ogólnie jeszcze nie wchodzę w żadne zagadnienia typu TDD. Sprawdziłem kod ręcznie z co najmniej 20 razy i działał dobrze więc ufam, że kod przynajmniej dobrze działa

MÓJ KOD : https://4programmers.net/Pastebin/15423

0

Już widzę dwa błędy. Nie zauważyłem ich. Literówka w tekście w cout - bez znaczenia. I zapomniałem zwolnić pamięci delete game;

2

Tworzysz obiekty na stercie, których nigdzie nie usuwasz ( potencjalne "memory leak" ).
Jeżeli tworzysz w konstruktorze obiekty.

    player = new Player;
    computer = new Player;

to dodaj kod w destruktorze który będzie je usuwał (RAII).

Najlepiej jednak kiedy zrezygnujesz z gołych wskaźników na rzecz inteligentnych.

    player = make_unique<Player>();
    computer = make_unique<Player>();

Zobacz https://cpp-polska.pl/post/kompendium-wiedzy-o-smart-pointerach , jeśli chcesz się dowiedzieć więcej.

1
  1. Wskaźniki w ogóle nie są tutaj potrzebne. To nie Java, że wszystko trzeba robić przez new :P
  2. Zamiast rand() skorzystaj z <random>
  3. Zapoznaj się z <algorithm>. Do "tasowania" czy kopiowania są już gotowe algorytmy
  4. Brakuje const-correctness
  5. Do ostatniego elementu wektora dostaniesz się poprzez metodę back(). Krócej i czytelniej niż [vec.size() - 1]
  6. Wszystkie metody zrobiłeś publiczne, mimo że niektóre są wywoływane wyłącznie wewnątrz klasy
  7. Przy rozdzielaniu kart rzuca się w oczy częste kopiowanie i usuwanie

I tak osobiście to nie przepadam za pętlami while(true) :P

2
  1. Masz sporo wycieków pamięci. Przeczytaj i zastosuj
  2. Liczby magiczne
  3. Zamiast deck[deck.size() - 1]; użyj deck.back().

Łatwiej by było jakbyś wystawił pull request na github/gitlab do oceny.

0
tajny_agent napisał(a):
  1. Wskaźniki w ogóle nie są tutaj potrzebne. To nie Java, że wszystko trzeba robić przez new :P
  2. Zamiast rand() skorzystaj z <random>
  3. Zapoznaj się z <algorithm>. Do "tasowania" czy kopiowania są już gotowe algorytmy
  4. Brakuje const-correctness
  5. Do ostatniego elementu wektora dostaniesz się poprzez metodę back(). Krócej i czytelniej niż [vec.size() - 1]
  6. Wszystkie metody zrobiłeś publiczne, mimo że niektóre są wywoływane wyłącznie wewnątrz klasy
  7. Przy rozdzielaniu kart rzuca się w oczy częste kopiowanie i usuwanie

I tak osobiście to nie przepadam za pętlami while(true) :P

Dzięki bardzo za rady. Poczytam o <random>. Dzięki też za radę odnośnie biblioteki na pewno z niej skorzystam. Potem też poczytam o const-correctness. Poprawię ten kod, faktycznie jest tu kilka metod, które powinny być prywatne. No i ta przejrzystość zamienię tą składnię na składnie korzystającą z funkcji back(). Faktycznie zmienię jakąś strukturę, żeby nie tyle nie kopiować. Czemu nie podoba ci się pętla while(true)

0
TomaszLiMoon napisał(a):

Tworzysz obiekty na stercie, których nigdzie nie usuwasz ( potencjalne "memory leak" ).
Jeżeli tworzysz w konstruktorze obiekty.

    player = new Player;
    computer = new Player;

to dodaj kod w destruktorze który będzie je usuwał (RAII).

Najlepiej jednak kiedy zrezygnujesz z gołych wskaźników na rzecz inteligentnych.

    player = make_unique<Player>();
    computer = make_unique<Player>();

Zobacz https://cpp-polska.pl/post/kompendium-wiedzy-o-smart-pointerach , jeśli chcesz się dowiedzieć więcej.

Dzięki bardzo za link z smart pointers. Ciekawa sprawa na pewno skorzystam

0
MarekR22 napisał(a):
  1. Masz sporo wycieków pamięci. Przeczytaj i zastosuj
  2. Liczby magiczne
  3. Zamiast deck[deck.size() - 1]; użyj deck.back().

Łatwiej by było jakbyś wystawił pull request na github/gitlab do oceny.

  1. Jak najbardziej skorzystam.
  2. Jakie liczby magiczne?
  3. Poprawię w kodzie

Wiem następnym razem wszystko wrzucę na githuba

0
lu7x00 napisał(a):
  1. Jakie liczby magiczne?

Np. w tej pętli:

    for(int i = 0; i < 4; i++)
    {
        for(int i = 2; i <= 14; i++)
            cards.push_back(i);
    }

Skąd się wzięły liczby 4, 2, oraz 14? Powinny być zadeklarowane jako stałe. Chyba szczególnie to boli przy powtarzanej wszędzie wartości 26.

Ja jeszcze dorzucę, że jeśli masz

 bool Game::isEnd()
{
    return isEndCards();
}

to chyba jedna z funkcji isEnd oraz isEndCards jest zupełnie zbędna.

  1. Błędy gramatyczne: Computer win war -> Computer wins the war (być może opcjonalnie bez the, ale s musi się pojawić).
  2. Wypisywanie na ekran w funkcji, która ma liczyć wynik (isEndCards()) - osobne funkcje powinny zajmować się logiką, a osobne prezentacją danych.
  3. w nowoczesnym C++ istnieje pętla zakresowa for. Twój kawałek
        for(int i = 0; i < winnerCards.size(); i++)
            computer->SaveToStack(winnerCards[i]);

można zapisać jako

        for(int card: winnerCards.size())
            computer->SaveToStack(card);

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