Conversation
… tak samo sposób określania preferowanych pól
…rawienia przypadek parzystej wysokości przy wyliczaniu preferowanych pól
…rawienia przypadek parzystej wysokości przy wyliczaniu preferowanych pól
… should write some tests
… takes two genoms and two ratios, that the algorithm creates new genome according to
…ws how many does it have xd
# Conflicts: # TheAbyss/src/main/java/agh/oop/project1/SimulationEngine.java
# Conflicts: # TheAbyss/src/main/java/agh/oop/project1/SimulationEngine.java
… for reading a collection, labels being in polish, null stats toString throwing exceptions)
Soamid
left a comment
Soamid
left a comment
There was a problem hiding this comment.
Funkcje:
1 / 1 wiele symulacji - śmiga
2 / 2 warianty - wszystko jest
0.8 / 1 konfiguracja + zapis wyników - są, ale konfiguracja nie jest w formie klucz=wartość, co jest mega niewygodne (i niebezpieczne).
2 / 2 wyświetlanie symulacji (kolorki, pauza, podglądanie) - wygląda pięknie i wygląda jakby działało, czego chcieć więcej. ;)
1 / 1 statystyki - brakuje tylko oznaczania wizualnie zwierzaków z dominującym genotypem, ale macie za to wizualne trackowanie więc też ok.
Kod:
3.8 / 4 Architektura
W niektórych miejscach szwankuje zasada SRP, szczególnie w przypadku SimulationEngine można by tam wydzielić dodatkowe klasy... ale tak sumarycznie to zrobiliście tu świetną robotę. Jesteście chyba jedynym zespołem (jak na razie), który porządnie zdekomonował część związaną z UI zamiast wrzucać wszystko do jednej klasy. Pojawiła się nawet próba wdrożenia wzorca MVC/MVP z tego co widzę (on trochę inaczej działa, ale to może opowiem na zajęciach jak będziemy rozmawiać). Także powinienem odjąć może ciut więcej, ale nadrobiliście w obszarach, w których tego nie oczekiwałem. ;)
2 / 2 Czystość kodu
Pod tym względem w ogóle elegancko. W pojedynczych miejscach brakuje hermetyzacji (uwaga na modyfikatory atrybutów), ale ogólnie kod czyta się bardzo dobrze, zastosowaliście też sporo "współczesnych" elementów Javy. W zasadzie projekt wygląda jak coś, co widuję na co dzień w pracy (a przynajmniej te lepsze fragmenty które widuję :D).
0.7 / 1 Obsługa błędów - tutaj średnio, bo obsługa nie jest zbyt przemyślana, a pliki nie są zamykane (patrz komentarze w kodzie).
Ogólnie rzecz biorąc jeden z najładniejszych projektów, jakie widziałem w tym roku, bardzo miło się to czytało i klikało. :)
Łącznie: 13.3 + 2 za intro = 15.3 za projekt
| protected Vector2d position; | ||
| protected MapDirection orientation; | ||
| private int date; | ||
| SimulationOptions simulationOptions; |
| public Animal(Vector2d initialPosition, IWorldMap map, Integer geneCount, int initialEnergy){ | ||
| this(initialPosition, map, new Genotype(geneCount), initialEnergy); | ||
| } | ||
| public Animal(Animal animal1, Animal animal2){ |
There was a problem hiding this comment.
ten i pierwszy konstruktor też można pożenić, zawsze warto zrobić jeden duży konstruktor dla wszystkich parametrów i pozostałe, które z niego korzystają (nie powinno być zduplikowanych przypisań).
| genes = new Genotype(animal1.getGenes(), animal2.getGenes(), animal1.getEnergy(), animal2.getEnergy()); | ||
| mutateGenes(); | ||
| animal1.subtractEnergy(map.getSimulationOptions().reproductionCost()); | ||
| animal2.subtractEnergy(map.getSimulationOptions().reproductionCost()); |
There was a problem hiding this comment.
tu mamy zaszytą logikę reprodukcji, która nie dość że jest "ukryta" w konstruktorze (pamiętajcie, że konstruktory to metody bez nazwy więc wszelka bardziej złożona logika powinna być z nich wydzielona do osobnej metody), to też nieszczególnie jest odpowiedzialnością zwierzątka - lepiej byłoby umieścić to w jakimś zewnętrznym mechanizmie albo w ostateczności w genotypie.
| } | ||
| @Override | ||
| public List<Vector2d> betterFields() { | ||
| return new ArrayList<>( betterFields.stream().toList() ); |
There was a problem hiding this comment.
można stworzyć listę na bazie zbioru, całe betterFields.stream().toList() niepotrzebne.
| } | ||
| private boolean placeFromFirstElsePlaceFromSecond(List<Vector2d> first, List<Vector2d> second) { | ||
| boolean found = false; | ||
| for (Vector2d vector2d : first) { |
There was a problem hiding this comment.
ten kod powtarza się dla obu przypadków, może po prostu zrobić metodę która przyjmuje jedną listę i próbuje wstawić roślinę. Wtedy trzeba by tę metodę zawołać dwa razy, ale i tak lepiej niż powtarzać kod.
| } | ||
| catch ( | ||
| IOException exception) { | ||
| exception.printStackTrace(); |
There was a problem hiding this comment.
przydałaby się jakaś realna obsługa błędu, w tym momencie jeśli np. pliku nie będzie to wypisze błąd na konsoli i program będzie próbował toczyć się dalej aż się wywali na jakimś NPE pewnie. Idealnie byłoby tu pokazywać jakiś Alert i próbę ponownego wczytania.
There was a problem hiding this comment.
Plik nie jest nigdzie zamykany --> polecam try with resources
| public LoadOptionsView getView() { | ||
| return view; | ||
| } | ||
| public void loadConfiguration (String path) { |
There was a problem hiding this comment.
podobnie jak w innych tego typu przypadkach: to nie jest odpowiedzialność prezentera by zarządzać wczytywaniem plików, wydzielić do osobnej klasy
| stringOptions.add(scanner.nextLine()); | ||
| }*/ | ||
| String[] stringOptions = scanner.nextLine().split(" "); | ||
| int[] intOptions = new int[12]; //cos jest nie tak z wielkoscia? |
There was a problem hiding this comment.
magiczny numerek, najlepiej wydzielić do nazwanej stałej... albo zaklepać to inaczej. Chciałem żeby konfiguracja była w formie wpisów klucz=wartość, wtedy samo wczytywanie też jest bezpieczniejsze, bo można zweryfikować czy są wszystkie wymagane klucze (opcje).
| //engine.await(); | ||
| //engine.pause(); | ||
| //engineThread.interrupt(); | ||
| engineThread.suspend(); |
There was a problem hiding this comment.
metody suspend()/resume() są deprecated, tj. nie powinniśmy ich używać, bo prawdopodobnie zostaną niebawem usunięte z Javy (warto czytać jdoc, tam jest ostrzeżenie i wyjaśnienie). Jeśli chcecie realnie blokować te wątki to polecam zamiast tego wait() / notify() lub jakiś mechanizm locków, pokazywaliśmy tego trochę na wykładzie.
| this.presenter = presenter; | ||
| } | ||
| //strasznie brzydkie | ||
| //przydalyby sie wyjatki krzyczace jak jest zly input |
3 participants
No description provided.