Conversation
SimultaionEngine commit
classes added + Vector2d&Direction implemented
classes & basic methods added
LittleAdjustment and TotalRandom implementation
class Animal implementation
IHolyGardener variants implementation
Behaviour and Map
Little adjustments
FileMenager, Statistics, Map
gui config continuation
last parts of gui planned
so many milestones
colors fixed
play/pause now working
Grass is being eaten, bugfixes
the app is working - we are the champions
legend fixed
last adjustments
Soamid
left a comment
Soamid
left a comment
There was a problem hiding this comment.
Funkcje:
1 / 1 wiele symulacji - da się odpalić kilka różnych.
2 / 2 warianty - wszystko jest!
1 / 1 konfiguracja + zapis wyników - jest!
2 / 2 wyświetlanie symulacji (kolorki, pauza, podglądanie) - wygląda ładnie, bardzo estetyczny róż i bardzo puchate futerka. Końce świata trochę depresyjne, ale co poradzić, taki mamy, ekhm, klimat.
1 / 1 statystyki - jest wszystko a nawet więcej niż się spodziewałem. :)
Kod:
4 / 4 Architektura
Jestem pod wrażeniem, pomimo tylu różnych pułapek udało Wam się to naprawdę dobrze poukładać. Czepiałbym się w kilku miejscach zasady SRP, ale to raczej niewielkie uwagi na tle całości i tego jak np. rozegrałyście wszystkie strategie, a nawet wprowadziłyście FXML, o którym nie mówiliśmy.
No i zarządzanie parametrami przypomina ręczną wersję profesjonalnego wstrzykiwania zależności, no jak złoto. Miałem odjąć za to SRP, ale wyszłyście znacznie ponad program więc nie odejmuję nic. ;)
1.7 / 2 Czystość kodu
Tu też bardzo ok, choć muszę się czepić jednego powtarzającego się problemu: hermetyzacja, a właściwie jej naruszanie. W Javie atrybuty są "niskopoziomowe" w przeciwieństwie do propertiesów w C# czy Kotlinie albo Scali. Musimy więc oznaczać go jako prywatne i ewentualnie dorabiać do nich gettery/settery w razie gdy są potrzebne.
Poza tym niektóre metody można by podzielić na mniejsze, a tak to spoko.
1 / 1 Obsługa błędów - jest i to całkiem realna, nawet w GUI się pokazują komunikaty. :) Jedynie uwaga do wyjąków unchecked (patrz komentarze w kodzie).
Łącznie: 13.7 + 1.8 za intro = 15.5 za projekt.
Jak na razie to rzeczywiście najpiękniejszy generator ewolucyjny, jaki widziała ludzkość, brawo. ;)
Podesłałbym jako nagrodę najlepszą piosenkę na świecie, ale po tylu godzinach sprawdzania nie pamiętam już, która jest najlepsza więc podeślę tylko: https://www.youtube.com/watch?v=_lK4cX5xGiQ
| private Map map; | ||
|
|
||
| //konstruktor dla Adama i Ewy | ||
| public Animal(SimulationVar var,Map map) { |
There was a problem hiding this comment.
konstruktory warto byłoby zwinąć tak by przypisania były robione tylko w jednym (prawdopodobnie trzeba by tu wprowadzić trzeci).
| private Map map; | ||
|
|
||
| //konstruktor dla Adama i Ewy | ||
| public Animal(SimulationVar var,Map map) { |
There was a problem hiding this comment.
var to bardzo niebezpieczna nazwa dla zmiennej, bo w Javie jest takie słowo kluczowe :P
| position=mommy.getPosition(); | ||
| genes= new Direction[var.getGenomeLength()]; | ||
| activeGeneIx=generator.nextInt(var.getGenomeLength()); | ||
| int energy1 = mommy.energy / 2; |
There was a problem hiding this comment.
ok, przy czym wyrzuciłbym logikę rozmnażania do osobnej metody, bo konstruktory z definicji są bezimienne i trzeba się domyślać co tu się dzieje właściwie.
| try { | ||
| createGenome(mommy, daddy, fromMommy); | ||
| } | ||
| catch (IllegalArgumentException exception){ |
There was a problem hiding this comment.
czy na pewno chcemy to tu łapać? w takiej sytuacji dziecko z niepoprawnie stworzonym genomem będzie dalej funkcjonować w aplikacji i przejdziemy do procedury mutacji :P
| if (genesToString().equals(map.getTheMostPopularGenome())){ | ||
| circle.setStroke(Color.valueOf("#fff705")); | ||
| } | ||
| circle.setOnMousePressed(new EventHandler<MouseEvent>() { |
There was a problem hiding this comment.
tu mamy wmieszany kod UI w model, tego unikajcie za wszelką cenę (to najgorszy przypadek złamania zasady jednej odpowiedzialności i zasady otwarte-zamknięte). Można by tu po prostu zwracać np. ścieżkę do obrazka albo stringa opisującego kolor, ale nie kontrolki FX.
| map.removeAnimal(animal); | ||
| } | ||
|
|
||
| //poruszamy wszystkie zwierzątka |
There was a problem hiding this comment.
tu ponownie fajnie byłoby wydzielić metody-etapy symulacji, np. moveAnimals(), dinnerTime(), weAllGonnaDie() itp. ;)
| } | ||
| possibleMatch.sort(Comparator.<Animal>comparingInt(el -> -el.energy).thenComparingInt(el -> -el.age).thenComparingInt(el -> -el.children)); | ||
| lastIndex = howManyOnThisSpot; | ||
| Animal winner = possibleMatch.get(0); //wygrał trawkę |
There was a problem hiding this comment.
mechanizm porównania i wybierania silnych zwierzątek wydzieliłbym w ogóle na zewnątrz, nawet do jakiejś klasy helpera, strasznie tu śmieci
| public void run() { | ||
|
|
||
| while(map.getStats().getAmountOfAnimals()>0 && !interrupted){ | ||
| Platform.runLater(()->{dayRitual();}); |
There was a problem hiding this comment.
to oznacza, że cały dayRitual() wykona się w wątku UI zamiast w wątku symulacji! Co czyni w sumie wątek symulacji bezużytecznym i zrzuca ciężar obliczeń na GUI.
| private final int grassSpawnedEveryday; | ||
| private final int refreshTime; | ||
| private final int genomeLength; | ||
| //bardzo brzydkie, aż nam wstyd:( |
There was a problem hiding this comment.
no, ale można to łatwo naprostować:
- Bazowanie na numerkach to zło, chciałem żeby ten config był w formacie "klucz=wartość", bo wtedy od razu widać w pliku co można ustawiać, ale też w kodzie łatwiej się to ogarnia. Można od razu wczytać do mapy wszystkie parametry, a potem...
- Dla każdego parametru stworzyć metodę providera, która zwraca odpowiedni obiekt strategii w przypadku tych złożonych parametrów typu rozdzaj mapy itp.
- Wydzielić wszystkie klucze do stałych
static final - Opcjonalnie można też rozdzielić wczytywanie i samo przechowywanie parametrów w dwóch osobnych klasach (z czego przechowywanie można by załatwić rekordem a nie klasą).
| for (Vector2d position: positionsIterator){ | ||
| if(!map.isGrassThere(position)){positions.add(position);} | ||
| } | ||
| positions.sort(new Comparator<Vector2d>() { |
There was a problem hiding this comment.
ok, ale można krócej lambdą
3 participants
No description provided.