Code Review #3
Code Review #3
Conversation
Added: Utils, Plant, Animal + Genes demo
Added: class structure + builder for WorldMap.
# Conflicts: # src/main/java/org/example/map/WorldMap.java # src/main/java/org/example/map/objects/plants/PlantsEquator.java # src/main/java/org/example/map/objects/plants/PlantsToxicCorpses.java
Soamid
left a comment
Soamid
left a comment
There was a problem hiding this comment.
Funkcje:
0.8 / 1 wiele symulacji - da się odpalić wiele, choć przyjęty sposób z brutalnym przerywaniem wątku może być zgubny i nie do końca jest poprawny.
1.8 / 2 warianty - wszystko jest, tylko nie obsłużyliście przypadków brzegowych gdy więcej zwierząt o tej samej energii jest na jednym polu.
1 / 1 konfiguracja + zapis wyników - jest ok
2/ 2 wyświetlanie symulacji (kolorki, pauza, podglądanie) - u mnie wysypywało się kodowanie znaczków do reprezentacji zwierząt, prawdopodobnie nie zapisywaliście tego poprawnie jako utf-8, ale tak poza tym to wygląda sensownie i da się zrobić wszystko co chciałem.
1 / 1 statystyki - są
Kod:
3.5 / 4 Architektura
Bardzo dobre zastosowanie obserwatorów, ma to wszystko razem sens. Granulacja klas też jest odpowiednia (nawet w UI, co się rzadko zdarza), szczególnie że wprowadziliście strategie do realizacji wariantów. To czego muszę się przyczepić to przeciążenie odpowiedzialnościami w klasie mapy, parę pomniejszych innych uwag tego typu w komentarzach.
1.8 / 2 Czystość kodu
- Trochę drobnych uwag z gatunku clean code, ale ogólnie kod wygląda bardzo dojrzale, czyta się to bardzo dobrze.
- Macie śmieci w repo, dosłownie - pliki .class, ogólnie cały katalog build powinien być w ignore.
0.7 / 1 Obsługa błędów - wyjątki się pojawiają, nawet jakaś walidacja za ich pomocą, ale niestety nie ma obsługi dla błędów IO - wypisuje się tylko błąd na konsoli i jest ciche założenie, że wszystko będzie dobrze jeśli plik nie jest poprawny.
Łącznie: 12.6 + 1 za intro = 13.6 za projekt
| * | ||
| * @author Thomas Bolz | ||
| */ | ||
| public class NumberTextField extends TextField { |
There was a problem hiding this comment.
to chyba nieużywane
| } | ||
|
|
||
| private void day() { | ||
| Statistics currentStats = map.day(); |
There was a problem hiding this comment.
nie jestem przekonany, czy realizacja wszystkich zadań dnia to odpowiedzialność modelu mapy, która powinna się zajmowac przede wszystkim przechowywaniem rzeczy i dbaniem o ich pozycje.
| Statistics currentStats = map.day(); | ||
| Optional<AnimalStatistics> animalStatistics = map.getTrackedAnimalStatistics(); | ||
|
|
||
| Platform.runLater(() -> { |
There was a problem hiding this comment.
a to można by połączyć akurat obserwatorem, podobnie jak na Lab8
| Button startSimulationButton = new Button("Pause"); | ||
| startSimulationButton.setOnMouseClicked(event -> { | ||
| if (engine != null && engine.isAlive()) { | ||
| engine.interrupt(); |
There was a problem hiding this comment.
nie jest to najbardziej elegancki sposób, bo możemy przerwać wątek w połowie jakichś obliczeń, które zostaną potem utracone.
| Platform.runLater(this::displayMap); | ||
| } | ||
|
|
||
| private void saveStatisticsToFile(File file) { |
There was a problem hiding this comment.
tu mamy wmieszaną obsługę plików w klasę zajmującą się UI - należałoby to wydzielić
| animalsIterator.forEach(Animal::removeIfDied); | ||
| } | ||
|
|
||
| public void eatPlants() { |
There was a problem hiding this comment.
od tego miejsca zaczynają się metody, które moim zdaniem nie są związane z odpowiedzialnością przechowywania obiektów mapy. Jedzenie i rozmnażanie to bardziej część symulacji (może dedykowane klasy?), a statystyki to też raczej serwis, który zna mapę i potrafi sobie z niej wyciągnąć wszystko co trzeba by policzyc.
| int genesLength) { | ||
| // Constructor creates genotypes for first animals ever placed on map | ||
| this.makeMove = makeMove; | ||
| this.genesLength = genesLength; |
There was a problem hiding this comment.
konstruktory powinny być zawsze zależne od siebie (można wprowadzić jakiś trzeci) tak by przypisania były tylko w jednym.
Btw, warto porządkować elementy klasy tak by na górze były stałe i atrybuty, potem konstruktory, a potem reszta metod.
|
|
||
| private synchronized Vector2d positionOutsideEquator() throws CannotPlacePlantException{ | ||
| if (!nonEquatorAccessible()) { | ||
| throw new CannotPlacePlantException("Can't place plant on non-preferred position"); |
There was a problem hiding this comment.
tu widzę, że sterujecie logiką programu przez wyjątki - to trochę w stylu pythona bardziej, nie jest to może złe, ale semantycznie te "checked exceptions" bardziej powinny służyć do obsługi problemów na linii program-użytkownik.
| default -> throw new UnsupportedOperationException("Edges' variant " + iEdge + " not implemented"); | ||
| }; | ||
|
|
||
| return new WorldMap(width, |
There was a problem hiding this comment.
tu trochę deja vu, to samo co w builderze... nie wiem który ostatecznie mechanizm ma tu działać, ale nie łączyłbym mapy i preferencji na takiej zasadzie, lepiej by jakiś provider przyjmował preferencje i tworzył na ich podstawie mapę.
| public void breeding() { | ||
| animals.values().forEach(animals -> { | ||
| if (animals.size() > 1) { | ||
| animals.sort(Comparator.comparingInt(Animal::getEnergy)); |
There was a problem hiding this comment.
a jeśli energie są równe? brakuje pozostałych przypadków
No description provided.