Ocena gry w stylu Candy Crush - C++ i SFML

pytanie zadane 19 sierpnia 2018 w Nasze projekty przez Michał Muzyka Pasjonat (24,080 p.)

Witam,
Przychodzę z prośbą oceny gry oraz kodu.

https://github.com/muzyczek/Diamonds

Link do pobrania(Windows):

https://drive.google.com/file/d/1-82fU6m1VZJcg65A-kgzau25bacF70tp/view?usp=sharing

2 odpowiedzi

odpowiedź 19 sierpnia 2018 przez profesorek96 Szeryf (91,420 p.)

Jak dla mnie grafika bardzo spoko, estetycznie wykonana gra.

komentarz 19 sierpnia 2018 przez Michał Muzyka Pasjonat (24,080 p.)

dziękuję

komentarz 19 sierpnia 2018 przez profesorek96 Szeryf (91,420 p.)

Z czego uczysz się SFML, jakie polecasz tutoriale ?

komentarz 19 sierpnia 2018 przez Michał Muzyka Pasjonat (24,080 p.)
edycja 20 sierpnia 2018 przez Michał Muzyka

W SFML zacząłem pisać jakoś ponad 2 lata temu, tylko z pewnych powodów dałem radę znów pisać dopiero teraz. A ogólnie to przez ten czas pisałem jakieś proste programiki, korzystałem z dokumentacji a różne triki i inne możesz znaleźć tutaj: https://github.com/SFML/SFML/wiki/Tutorials i tutaj: https://github.com/SFML/SFML/wiki/Sources spoko jest też książka SFML essentials

komentarz 20 sierpnia 2018 przez profesorek96 Szeryf (91,420 p.)

O widzisz wielkie dzięki :)

odpowiedź 20 sierpnia 2018 przez monika90 Pasjonat (22,940 p.)
edycja 20 sierpnia 2018 przez monika90

Wszędzie singletony, jakieś managery, po co są te klasy? Mogłoby ich nie być - program byłby krótszy i też by działał. Twoja gra używa jednej czcionki, ale masz font managera, jakby to był co najmniej Adobe PageMaker.

I ten singleton Settings - używasz globalnej tablicy asocjacyjnej string->string do przekazywania parametrów numerycznych z jednej części programu do drugiej?

Przekazujesz typy całkowitoliczbowe przez referencję, nawet bool, dlaczego? Tak się nie robi, to może zmniejszyć wydajność z powodu aliasingu.

Pragma once jest niestandardowa, nie wiadomo co robi i nie jest potrzebna bo klasyczne include guards wystarczają.

Random_int - klasa która ma tylko jedną funkcję i to statyczną. Dlaczego to jest klasa? Mogłaby być po prostu funkcja.

komentarz 20 sierpnia 2018 przez Michał Muzyka Pasjonat (24,080 p.)

wydawało mi się że przekazywanie wartości przez stałą referencję jest jak najbardziej ok i jest dobrą praktyką: https://softwareengineering.stackexchange.com/questions/372105/is-passing-arguments-as-const-references-premature-optimization

te singletony i managery pisałem na potrzeby innego projektu i tak zostały,

pragama once - no visual sam ją dodaje to zostawiałem

no z tym random_int no to tak rzeczywiście nie przemyślałem trochę

komentarz 20 sierpnia 2018 przez monika90 Pasjonat (22,940 p.)
edycja 20 sierpnia 2018 przez monika90

W przypadku np. std::string przekazywanie przez referencję to pewnie jest dobra praktyka, w przypadku int, size_t, i bool na pewno nie jest.

Nie tylko ja tak uważam, reguła F.16: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-in

komentarz 20 sierpnia 2018 przez Michał Muzyka Pasjonat (24,080 p.)

ok, dziękuję nie wiedziałem :)

komentarz 20 sierpnia 2018 przez mokrowski Mędrzec (155,460 p.)

Super że chcesz oceny :) Nic tak nie uczy jak "zdrowe czepialstwo" innych :)

Na szybko...

*.dll'e SFML w repozytorium to zła praktyka. Takie obiekty nie są potrzebne chyba że mocno wiążesz się z daną wersją lub jest to SFML rekompilowany z jakimiś egzotycznymi opcjami. I tak w takich przypadkach chyba lepiej informować o samych opcjach bo ktoś będzie (być może) uruchomić Twoją grę np. na GNU/Linux

Kolejność wczytywania nagłówków to raczej: systemowe/biblioteczne, te z projektu. Zawsze jest szansa że te z projektu mogą zmieniać nieco konfigurację tych systemowych. Odwrotnie raczej nie.

Nie szalej z referencjami. Nawet std::string często ma optymalizację kopiowania do 16 znaków (na głównych platformach), jako szybszą niż referowanie. Referencje do POD'ów bool i int to raczej zły pomysł @monika90 już to udokumentowała...

set_sprite_position i move_sprite... hmm.. trochę wygląda na duplikację funkcjonalności. W połączeniu z set_position daje jeszcze większe zastanowienie o co chodzi?

W listach inicjalizacyjnych preferuj nawiasy klamrowe. Okrągłe zostaw dla wyrażenia przypadku zamierzonej konwersji.

Często łamiesz Regułę Demeter (nie rozmawiaj z obcymi). Masz pociągi wywołań w stylu func1(Object2->func2().func3()). Będzie trudniej utrzymywać taki kod choć to mały projekt.

Co pragma once, raczej wiadomo co robi. Oczywiście to pragma a ona jako taka jest poza standardem ale bywa że przyśpieszy kompilację bo nie trzeba zajmować "lichej" (bo jednej) przestrzeni nazewniczej makr definicjami strażników i dodatkowo kompilator nie otworzy pliku nagłówka tylko po to by stwierdzić że .. ma go nie parsować bo strażnik jest zdefiniowany. IMHO to mniejszy problem w tym przypadku. Wystarczy świadomość że wiąże się z konkretnym kompilatorem. Choć... jaki kompilator (głównego nurtu) nie ma tej pragmy lub jej odpowiednika?

A co do kodu na szybko... nawet nadanie samej nazwy "Manager", "Procesor", powoduje z mojego doświadczenia szybkie łamanie SRP w większym projekcie. Każdy chce tam wcisnąć coś od siebie. Powstaje wtedy boska klasa.

Singleton w zasadzie w C++ jest nieużywalny. Abstrahując już od jego sensu używania (bo nie ma sensu).

Przemyślał bym także animację. Z samych definicji elementów ruchomych, wynika że mają wydzielone przesuwanie i animowanie tego przesuwu (nie analizowałem dokładnie więc mogę się mylić).

Co do ranom, podpowiedź. Czasem w projektach przydaje się "zakłamanie tej funkcji" tak aby generowała określony ciąg wartości. Łatwiej wtedy testować określone zachowania aplikacji bo są powtarzalne. Także zgadzam się że to trochę rozdmuchane by do tego tworzyć klasę.

Jesteś pewien że niektórych konstrukcji nie da się implementować z użyciem <algorithm>? Jeśli widzę 3 i 4 zagnieżdżenie w kodzie, to chyba się coś dzieje tak z samym kodem jak i jego modularyzacją.

Stosuj deklaracje zapowiadające. Nie zawsze w nagłówkach potrzebne jest włączanie innych. Szczególnie jeśli argument jest wskaźnikiem lub referencją lub argumentem szablonu.

To takie pobieżne uwagi bez wnikania w same algorytmy. Co do reguł, zawsze ktoś ma własne preferencje i nie zawsze kategoryzacja czarne/białe jest odpowiednia. Każde łamanie reguły ma swoje tak zalety jak i wady.

komentarz 20 sierpnia 2018 przez Michał Muzyka Pasjonat (24,080 p.)
edycja 20 sierpnia 2018 przez Michał Muzyka

Dzięki wielkie za uwagi :) Może kolejny projekt będzie lepszy.

Jednak, mam kilka pytań odnośnie:

" Masz pociągi wywołań w stylu func1(Object2->func2().func3()). " - nie za bardzo wiem o jakie funkcje tu chodzi.

Animation_manager::instance()->get(std::to_string(i) + "_s.png")->set_sheet(Texture_manager::instance()->get(std::to_string(i) + "_s.png"));

O coś takiego?

Jeśli nie singletony i nie managery to jak przechowywać tekstury w jednym miejscu? Czy lepiej jest za każdym razem przekazywać te tekstury poprzez wskaźnik do danego obiektu? Tak samo z ustawieniami.

komentarz 20 sierpnia 2018 przez mokrowski Mędrzec (155,460 p.)

Wiesz... chodzi o to że projekty "znieczulasz" na odpowiednie osie zmian które przewidujesz lub dodajesz "techniczne funkcjonalności" które mogą być użyteczne. Problem w tym że nie da się "znieczulić projektu na wszystkie osie zmian" bo wprowadzał byś obiekty oraz warstwy abstrakcji które doprowadziły by w konsekwencji do degradacji wydajności lub powodowały by dużą komplikację kodu albo (jak to w syndromie krótkiej kołdry gdzie przykryjesz nos a wystają nogi), naruszył byś rzeczy dla Ciebie ważniejsze. Tak więc ideał umyka jak króliczek:) Inna sprawa to ta że świadomie pewnych właściwości nie implementujesz bo nie są potrzebne. To czy się mylisz i czy trafiasz w przewidywaniach to już inne zagadnienie.

Animation_manager::instance()->get(std::to_string(i) + "_s.png")->set_sheet(Texture_manager::instance()->get(std::to_string(i) + "_s.png"));

Właśnie o takie zjawiska chodzi.

Jeśli masz w kodzie o.func1().func2().func3() to kompilator powinien wiedzieć że to co zwraca o.func1() posiada metodę func2() a to co ona zwraca ma func3(). Ta wiedza powoduje duże sprzężenie kodu i trudność zmiany implementacji. Zaczynasz mieć konieczność zaglądania przy zmianie w dziwne miejsca które nie są nijak usprawiedliwione. Druga sprawa to "budowanie argumentu" w wywołaniu. Takie nazwy z esem na końcu są raczej domeną konfiguracji a nie wklepywania "na twardo w kod". A jak zechcesz zmienić konwencję? Teraz ma być _x ? Miejsc do poprawy będzie wiele.

Zamiast singletona: Multiton, Dependency Injection, Non-Virtual Interface Idiom... To bardzo zależy od kontekstu co chcesz uzyskać. Poczytaj o tych idiomach/wzorcach/technikach.

Co do nazw "Manager", "Procesor", chciałem wyrazić to że działają jak magnes. Nawet Texture_Mapper jako nazwa była by pod tym względem lepsza niż Manager. To się wydaje trochę drobiazgowe ale nazwa to w większym projekcie gdzie dużo ludzi koduje rzecz istotna a ja powiedziałem o swoich doświadczeniach. Może inni się nie zgodzą i także będzie ok.. :)

komentarz 20 sierpnia 2018 przez Michał Muzyka Pasjonat (24,080 p.)

Okej chyba rozumiem, jeszcze raz dziękuję :)

o.func1().func2().func3();

rozbicie czegoś takiego żeby było coś takiego, też chyba nie będzie miało większego sensu:
a = o.func1();

b = a.func2();

b.func3();

bo dalej zostają nam te same zależności tylko że podzielone na kilka linijek
najlepiej doprowadzić to do wywołania tylko jednej funkcji?

komentarz 20 sierpnia 2018 przez mokrowski Mędrzec (155,460 p.)

Zasada "nie proś o dane do wykonania tylko poproś kogoś by wykonał to za Ciebie".

Jeśli rozbijesz na a = ... , b = ... problemu nie rozwiązujesz a tylko "owijasz w ładny papierek". To jest problem struktury...

BTW. Właśnie sprawdzam budowanie projektu na innym kompilatorze. Są błędy struktury, wczytywania nagłówków.... Jak skończę wystawię zip do skopiowania...

komentarz 20 sierpnia 2018 przez mokrowski Mędrzec (155,460 p.)

Nie zmieniałem struktury projektu, w większości plików poprawiłem jedynie deklaracje zapowiadające oraz kolejność nagłówków. Z widocznych przykładów powinieneś się zorientować co i jak:

https://www.dropbox.com/s/h0ij9mgrngog245/Diamonds-master.zip?dl=0