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

Sprawdzenie kodu w C++

Odpowiedz Nowy wątek
2020-03-19 13:18

Rejestracja: 8 miesięcy temu

Ostatnio: 11 godzin temu

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

Pozostało 580 znaków

2020-03-19 13:31

Rejestracja: 8 miesięcy temu

Ostatnio: 11 godzin temu

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;

edytowany 1x, ostatnio: lu7x00, 2020-03-19 13:31

Pozostało 580 znaków

2020-03-20 08:29

Rejestracja: 4 lata temu

Ostatnio: 57 minut temu

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.

Pozostało 580 znaków

2020-03-20 13:02

Rejestracja: 6 lat temu

Ostatnio: 1 tydzień temu

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


#duda2020

Pozostało 580 znaków

2020-03-20 13:29

Rejestracja: 12 lat temu

Ostatnio: 1 minuta temu

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.


Jeśli chcesz pomocy, NIE pisz na priva, ale zadaj dobre pytanie na forum.
edytowany 1x, ostatnio: MarekR22, 2020-03-20 13:30

Pozostało 580 znaków

2020-03-20 17:46

Rejestracja: 8 miesięcy temu

Ostatnio: 11 godzin temu

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)

Pozostało 580 znaków

2020-03-20 17:47

Rejestracja: 8 miesięcy temu

Ostatnio: 11 godzin temu

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

Pozostało 580 znaków

2020-03-20 17:48

Rejestracja: 8 miesięcy temu

Ostatnio: 11 godzin temu

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

Pozostało 580 znaków

2020-03-21 02:19

Rejestracja: 6 lat temu

Ostatnio: 7 godzin temu

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

1)

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

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

2) Błędy gramatyczne: Computer win war -> Computer wins the war (być może opcjonalnie bez the, ale s musi się pojawić).
3) Wypisywanie na ekran w funkcji, która ma liczyć wynik (isEndCards()) - osobne funkcje powinny zajmować się logiką, a osobne prezentacją danych.
4) 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);
edytowany 4x, ostatnio: enedil, 2020-03-21 02:27

Pozostało 580 znaków

Odpowiedz

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