Added: Animal, AnimalTree, Genotype, GenotypeTests. #3
Conversation
… make findMostCommonGenotype() dynamic, tests for Animal class. Work in progress
| import agh.ics.oop.model.worldMaps.Globe; | ||
|
|
||
| public class Animal { | ||
| private final Globe globe; |
There was a problem hiding this comment.
There is no reason to keep Globe in Animal.
You use it to:
- Get specific values in constructor - they can just be passed
- Check genotype length - you can pass it to Genotype as attribute or check the length of parent's genotype
| public Genotype(Animal mother, Animal father) { | ||
| validateParents(mother, father); | ||
|
|
||
| int divisionPoint = (int) (((double) mother.getEnergy() / (father.getEnergy() + mother.getEnergy())) * mother.getGenotype().genes.size()); |
There was a problem hiding this comment.
You access genotype length twice in this constructor. Imo it would be better to either add an attribute for it or create a local variable to make code more readable.
| private static final Random random = new Random(); | ||
|
|
||
| public static int getRandomInt(int bound) { | ||
| return random.nextInt(bound); |
There was a problem hiding this comment.
This method is exclusive but it's overload with 2 variables is inclusive. It will lead to confusion and mistakes in the future.
| } | ||
| } | ||
|
|
||
| public void switchGenes(int firstGeneIndex, int secondGeneIndex) { |
There was a problem hiding this comment.
You can use Collections.swap() to do this.
| return genotypeToString.toString(); | ||
| } | ||
|
|
||
| public Integer getCurrentGene(int currentGeneIndex) { |
There was a problem hiding this comment.
In this case it is better to use int instead of Integer.
| @@ -21,7 +21,9 @@ public class Globe { | |||
| private final int energyUsedToReproduce; | |||
| private final int minNumberOfMutations; | |||
| private final int maxNumberOfMutations; | |||
| private final int genomeLength; | |||
| private final int genotypeLength; | |||
| private final Map<Genotype, Integer> genotypeCount = new HashMap<>(); | |||
There was a problem hiding this comment.
You are using a map but there is neither equals nor hash in Genotype
| Vector2d position = animal.getPosition(); | ||
| List<Animal> animalsAtThisPosition = animals.get(position); | ||
|
|
||
| if (animalsAtThisPosition != null) { |
There was a problem hiding this comment.
I would replace it with try catch because we expect that position in animal is kept correctly.
| private void removeGenotype(Animal animal) { | ||
| Genotype animalGenotype = animal.getGenotype(); | ||
|
|
||
| genotypeCount.put(animalGenotype, genotypeCount.getOrDefault(animalGenotype, 1) - 1); |
There was a problem hiding this comment.
Same as above we expect that genotypes are also tracked correctly so no need for getOrDefault().
| genotypeCount.remove(animalGenotype); | ||
| } | ||
| } | ||
| public Genotype findMostCommonGenotype() { //niedynamiczne- do poprawy |
| return children.size(); | ||
| } | ||
|
|
||
| public int getDescendantsCount() { |
There was a problem hiding this comment.
My idea is that once you choose a specific animal, you get all of its children and add an observer to them that updates out count whenever a new animal is born (from an observed animal).
Not sure if it is the correct solution.
Things to change: make findMostCommonGenotype() dynamic, make AnimalTree more dynamic, check if AnimalTree class is even needed- maybe we could squish it into Animal class?, tests for Animal class. Work in progress...