Hmja, eine explizite „Best Practice“, ob man resources in
resources/com/bla/image.png
oder
com/bla/resources/image.png
oder
sonstwohin legen sollte, kann ich daraus zwar nicht ableiten, aber vermutlich muss man das von Fall zu Fall entscheiden…
Diese „rollOtherItems“-Methode sieht grauslig aus. Vorneweg: Sie ist, soweit ich das sehe, von Grund auf „fehlerhaft“, im Sinne von nicht total korrekt. Schon eine Schleife wie
while (third.equals(first))
third = getRandomItem();
}
könnte unendlich lange laufen. Es ist (so, wie die Implementierung aussieht) nicht garantiert, dass überhaupt mal ein Item gewählt wird, das NICHT gleich dem ersten Item ist. (Im vordergründigsten Fall: Die erste Schleife in deinem Code würde unendlich lange laufen, wenn es nur ZWEI Items gäbe. Dass bei „vielen“ Items die Wahrscheinlichkeit, dass sie unendlich lange läuft, verschwindend gering ist, ändert daran nichts).
In den while-Bedingungen sind ja mehrere Kriterien codiert. Die erste scheint zu sein: Alle fünf Items müssen unterschiedlich zueinander sein. Dafür gibt es geschicktere Lösungen, aber das ist auch noch der einfachste Fall. Die anderen Kriterien sind etwas komplizierter, und „domänenspezifisch“. Ich könnte jetzt anfangen, zu versuchen, diese Kriterien in Worten aufzuschreiben:
fourth.equals(second) || fourth.equals(third) || fourth.equals(first)
: Alle Items müssen unterschiedlich sein
fourth.isGoldItem() && third.isGoldItem() || fourth.isGoldItem() && second.isGoldItem()...
: Es darf immer nur EIN Gold-Item ausgewählt werden
fourth.isRangedOnly() && !currentChampion.isRanged()
: Wenn der currentChampion nicht „ranged“ ist, darf man kein „ranged only“ Item wählen
fourth.isMeeleeOnly() && currentChampion.isRanged()
: Wenn der currentChampion „ranged“ ist, darf man kein „melee only“ Item wählen
(Die Frage, wie sichergestellt ist, dass diese Kriterien schon für die ersten beiden Items erfüllt sind, mal außen vor gelassen)
Es erscheint (soweit man das beurteilen kann) geschickter, das Pferd andersherum (also von vorne
) aufzuzäumen: Es gibt eine Menge von „Items“, und nur wenige davon sind „gültig“, entsprechend der aufgezählten Kriterien. Es wäre dann IMHO viel einfacher, viel eleganter, viel übersichtlicher, und insbesondere auch viel leichter wartbar und testbar (!), sich die „gültigen“ Items als solche manifestieren zu lassen.
Ganz konkret heißt das (anskizziert, um die Idee zu verdeutlichen)
// Erstelle die Menge aller "gewählten" Items - am Anfang nur "first" und "second"
Set<Item> chosenItems = new HashSet<Item>(Arrays.asList(first, second));
while (chosenItems.size() < 5)
{
// Erstelle eine Menge der "gültigen" Items - am Anfang sind das ALLE:
Set<Item> validItems = new HashSet<Item>(items);
// Die, die schon gewählt sind, sind schon nicht mehr "valid":
validItems.removeAll(chosenItems);
// Wenn schon ein "Gold"-Item gewählt wurde, sind weitere "Gold"-Items nicht mehr zulässig
if (isGoldItemContainedIn(chosenItems))
{
removeAllGoldItemsFrom(validItems);
}
// Wenn der currentChampion nicht "ranged" ist, sind "ranged-only"-Items nicht zulässig
if (!currentChampion.isRanged())
{
removeAllThatAreRangedOnlyFrom(validItems);
}
// Wenn der currentChampion "ranged" ist, sind "melee-only"-Items nicht zulässig
if (currentChampion.isRanged())
{
removeAllThatAreMeleeOnlyFrom(validItems);
}
// Erstelle eine Liste
List<Item> list = new ArrayList<Item>(validItems);
// Keine gültigen Items mehr übrig? Fehler!
if (list.isEmpty()) reportSomeError();
// Wähle das nächste, gültige element, zufällig:
Item next = list.get(random.nextInt(list.size());
chosenItems.add(next);
}
Achtung: Da sind einige Hilfsmethoden angedeutet, z.B. sowas wie removeAllThatAreRangedOnlyFrom(someSet)
. Die sind zur Verdeutlichung gedacht. Auch wenn ihre Implementierung recht trivial wäre, würde man (wenn man es „gut“ machen wollte) diese Methoden NICHT so implementieren, sondern verallgemeinern.
Genaugenommen würde sich für die ganze Funktion anbieten, das mit den neuen Java 8 Streams zu machen. Ich weiß nicht, ob du die schon verwenden kannst oder willst, aber damit könnte man diese Kriterien ganz straighforward als „Predicates“ schreiben, und das ganze würde auf wenige, übersichtliche Zeilen zusammengedampft werden. Das könnte dann (GROB, nur aus dem Kopf schnell hingeschrieben) etwa so aussehen:
Set<Item> chosenItems = new HashSet<Item>(Arrays.asList(first, second));
while (chosenItems.size() < 5)
{
Stream stream = Stream.of(items);
stream = stream.filter(i -> !chosenItems.contains(i));
if (Stream.of(chosenItems).anyMatch(Item::isGoldItem)) stream = stream.filter(i -> !i.isGoldItem());
if (!currentChampion.isRanged()) stream = stream.filter(i -> !i.isRangedOnly());
if ( currentChampion.isRanged()) stream = stream.filter(i -> !i.isMeleeOnly());
List<Item> list = stream.collect(Collectors.toList());
if (list.isEmpty()) reportSomeError();
Item next = list.get(random.nextInt(list.size());
chosenItems.add(next);
}
(Komplett ungetestet, nur um den Punkt zu verdeutlichen)