Abstrakte Klasse und protected Konstruktor

viele Vorschläge hier, da kann ja einer mehr nicht schaden:
wenn man getResourceAsStream auf eine bestimme Klasse nutzt, dann ja oft, um einen Pfad relativ dazu anzugeben

falls also das resources-Verzeichnis im passenden Verzeichnis des packages liegt, dann teste auch
getResourceAsStream("resources/icon.png");

ganz ‘com/youtube/mrschesam’ usw. aufzuführen, ob korrekt oder nicht gewiss mit / am Anfang, das ist doch eher unschön?
dann schon eher der auch genannte Weg, resources nach oben, ins Grundverzeichnis zu verschieben

        String s = "com" + f + "youtube" + f + "mrschesam" + f + "ultimatebravery" + f + "resources" + f + "icon.png";
        String s2 = "\\resources\\icon.png";
        String s3 = "com/youtube/mrschesam/ultimatebravery/resources/icon.png";
        String s4 = "resources/icon.png";
        System.out.println(s);
        
        InputStream stream = BraveryDemo.class.getResourceAsStream(s);
        InputStream stream2 = BraveryDemo.class.getResourceAsStream(s2);
        InputStream stream3 = BraveryDemo.class.getResourceAsStream(s3);
        InputStream stream4 = BraveryDemo.class.getResourceAsStream(s4);
        System.out.println("Stream: " + stream);
        System.out.println("Stream2: " + stream2);
        System.out.println("Stream3: " + stream3);
        System.out.println("Stream4: " + stream4);
        
        try {
            Image image = ImageIO.read(stream4);
            System.out.println(image);
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }```

```com\youtube\mrschesam\ultimatebravery\resources\icon.png
Stream: null
Stream2: java.io.BufferedInputStream@2a139a55
Stream3: null
Stream4: java.io.BufferedInputStream@15db9742
BufferedImage@76fb509a: type = 5 ColorModel: #pixelBits = 24 numComponents = 3 color space = java.awt.color.ICC_ColorSpace@300ffa5d transparency = 1 has alpha = false isAlphaPre = false ByteInterleavedRaster: width = 16 height = 15 #numDataElements 3 dataOff[0] = 2```

Also Slaters Vorschlag ist der einzige der funktioniert in dem Beispiel, also entweder escapte Backslashe oder normale Slashes. Ich probiere das noch kurz im Projekt aus.

Edit: Slaters Beispiel funktioniert jetzt auch im Hauptprojekt, also ich kann die jar jetzt ausführen. Danke!

Nur der Vollständigkeit halber, nochmal mein letzter Hinweis:

String s5 = "/com/youtube/mrschesam/ultimatebravery/resources/icon.png";

!?

Das sollte auch gehen. Ob es eine “best practice” gibt, in bezug auf die Frage, ob der Pfad komplett angegeben, oder, OHNE führendes /, relativ zur Klasse, wie SlaterB gesagt hat, weiß ich nicht - insbesondere auch, weil ich nicht weiß, ob es überhaupt “üblich” ist, solche resourcen in dieser Form in packages zu legen. Wenn dieses “icon.png” jetzt auch von einer Klasse “com.youtube.mrschesam.ultimatebravery**.impl**.Foo” oder “com.youtube.mrschesam**.somethingelse**.Util” benötigt wird, wie macht man das dann am besten? Ich hielt es bisher für “üblich”, einfach einen “resources”-Folder in der JAR-root zu haben, und dann eben mit /resources/foo.png darauf zuzugreifen (dass man das darin noch unterteilen kann, nach “icons”, “images”, “sounds”, “maps” etc, ist ja klar). Aber wenn man sehr viele verschiedene, strikt package-spezifische resourcen hat, kann man sicher auch den packagenamen dort unterbringen. (DANN könnte man noch
/resources/com/mysite/icon.png oder
/com/mysite/resources/icon.png
in Betracht ziehen, aber das muss man sich dann überlegen…)

Smartly load your properties | JavaWorld

Class.getResourceAsStream ("/some/pkg/resource.properties");

Additionally, if the code is in a class within a some.pkg Java package, then the following works as well:

Class.getResourceAsStream ("resource.properties");

Gilt fuer alle Arten von Ressourcen, nicht nur Properties oder Bilder

Alles klar, danke. Hab es in den Root Ordner verlegt. Ich kannte es nur so, das man alles in das Projekt-Package legt.

Edit: Ich hab dann noch ne Frage zur Leserlichkeit:
Also mein Programm läuft und funktioniert jetzt auch ohne Probleme.
Aber ich habe 3 Methoden, die die Items anzeigen.
In der 3. wird innerhalb von 3 Schleifen Fälle getestet, die nicht vorkommen sollen. Also z.B. sollte kein Item 2x vorkommen und ein Fernkämpfer sollte kein Nahkampf-Item bekommen.
Dann mal der Code:

        Item first = currentFirstItem;
        Item second = currentSecondItem;
        Item third = getRandomItem();
        Item fourth = getRandomItem();
        Item fifth = getRandomItem();

        while (third.equals(second) || third.equals(first)
                || third.isGoldItem() && second.isGoldItem() || third.isGoldItem() && first.isGoldItem()
                || third.isRangedOnly() && !currentChampion.isRanged() || third.isMeeleeOnly() && currentChampion.isRanged()) {
            third = getRandomItem();
        }

        while (fourth.equals(second) || fourth.equals(third) || fourth.equals(first)
                || fourth.isGoldItem() && third.isGoldItem() || fourth.isGoldItem() && second.isGoldItem() || fourth.isGoldItem() && first.isGoldItem()
                || fourth.isRangedOnly() && !currentChampion.isRanged() || fourth.isMeeleeOnly() && currentChampion.isRanged()) {
            fourth = getRandomItem();
        }

        while (fifth.equals(second) || fifth.equals(fourth) || fifth.equals(third) || fifth.equals(first)
                || fifth.isGoldItem() && fourth.isGoldItem() || fifth.isGoldItem() && third.isGoldItem() || fifth.isGoldItem() && second.isGoldItem() || fifth.isGoldItem() && first.isGoldItem()
                || fifth.isRangedOnly() && !currentChampion.isRanged() || fifth.isMeeleeOnly() && currentChampion.isRanged()) {
            fifth = getRandomItem();
        }

        currentThirdItem = third;
        currentFourthItem = fourth;
        currentFifthItem = fifth;
    }

    private Item getRandomItem() {
        return items.get(r.nextInt(items.size()));
    }```

Also sie funktioniert, ich habe bei vielen Tests keinen Fall gefunden, bei dem etwas durchgerutscht ist. Allerdings sind das sehr viele Bedingungen in den Schleifen, besonders der 3. Ich habe durch Absätze das ganze schon leserlicher gemacht, aber ich denke mal, es gibt keine viel leserliche Methode dafür?
Hab den Source mal in den Anhang gepackt, wer es sich dort anschauen möchte. In der MainFrame-Klsse kann man nach dem Konstruktor alles bis Zeile 526 überspringen, da dort nur die Listen befüllt werden und die initComponents()-Methode dort ihr DIng verrichtet.

Komplizierte Bedingungen packt man am besten in eine extra Methode. Dann bleibt der aufrufende Code leserlich.

Bei deinem Spiel(?) wäre sicher noch andere Ansätze denkbar, im Moment sieht es wirklich schwer erweiterbar aus. So wie es im Moment aussieht, schreit es nach zukünftigen copy & paste Fehlern und macht sicher keine Freude, wenn man die Logik ändern will.

Da könntest du dir irgendwas schlaues mit Klassen, Listen etc. einfallen lassen.

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 :wink: ) 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)

etwas arg aufwendig mit Streams usw., bis dieser Code als übersichtlich gilt und nicht nur von Experten gepostet wird mag noch manches Jahr vergehen,
und wieviele Items sind vorhanden, zigmal die ganze Liste kopiert und durchlaufen…,
bei theoretisch zufälliger Neuerzeugung ganz aufgeschmissen

ist es eigentlich ein Problem wenn viel Logik an allgemeine Stelle muss?
ob einem ‚Champion‘ ein Item/ Waffe recht ist, wäre sonst evtl. gute innere Methode, Prinzip ‚Tell, Don’t Ask‘ oder so :wink:

eine Variante mit klassischeren Mitteln noch zur Ansicht:

chosenItems.add(first);
chosenItems.add(second);
while (chosenItems.size() < 5)
{
    Item next = getRandomItem();
    boolean accept = currentChampion.isSuitable(next);
    for (Item item : chosenItems) {
         if (item.equalOrBothGold(next) {
              accept = false;
              break;
         }
    }
    if (accept) {
        chosenItems.add(next);
    }
}

currentThirdItem = chosenItems.get(2);
usw. 

das Zufallsproblem besteht natürlich allgemein theoretisch, aber eins mit dem man sicher leben kann

Nun, das ganze wird nur einmal durchlaufen, am Ende, und dann werden alle Bedingungen geprüft. Die Wahl zwischen
stream.filter(A).filter(B).toList(); und
stream.filter(A.and(B)).toList();
ist auch noch offensichtlich.

Ob es in der Entscheidungsgewalt des „champions“ liegen sollte, feszulegen, welche Items gültig sind, darüber kann man streiten, im „TellDontAsk“ Sinne sicher, aber selbst wenn man das so macht, kann man zumindest das Zufallsproblem ja noch analog lösen:

for (Item item : items)
{
    if (currentChampion.isSuitable(item)) suitableItems.add(item);
}

// Wie gehabt...
suitableItems.removeAll(chosenItems);
List<Item> list = new ArrayList<Item>(suitableItems);
if (list.isEmpty()) reportError();
Item next = list.get(random.nextIt(list.size());
...

oder eben

Stream<Item> suitableItems = Stream.of(items).filter(i -> currentChampion.isSuitable(i));
...

:wink: