Code-Snippet

Bad Smells: Mag jemand refactoren? Ist recht ungewöhnlich, was ich da hab:

        for (boolean weiter = true; weiter;) {
            setItems();
            items.sort(new Comparator<Item>() {
                @Override
                public int compare(Item o1, Item o2) {
                    return o1.dauer - o2.dauer;
                }
            });
            for (Item item : items) {
                if (item.verfuegbar) {
                    click(item.getClickableElement());
                    weiter = true;
                    break;
                } else {
                    weiter = false;
                }
            }
        }

Danke für euren Input.

Und der Bug ist jetzt genau… was?

[edit SlaterB: Beiträge verschoben aus einem Thema zu Bugs]

1 „Gefällt mir“

Mal aus der Hüfte und ohne Gewähr (ist schwer ohne Kontext):

 //könnte noch verbessert werden, je nach Typ von dauer
Comparator<Item> comparator = (o1, o2) -> o1.dauer - o2.dauer;
Optional<Item> verfuegbaresItem; 
do {
    setItems();
    items.sort(comparator);
    verfuegbaresItem = items.stream().filter(item -> item.verfuegbar).findFirst();
    verfuegbaresItem.ifPresent(item -> click(item.getClickableElement()));  
} while(verfuegbaresItem.isPresent())

Natürlich wäre Code ohne Seiteneffekte schöner. Und Denglisch würde ich auch vermeiden.

Dankeschön. Genau das war die Intention dahinter. :wink:
Mehr zum Kontext kann ich schreiben, wenn wünscht.
Aaaaber leider hast du mit ifPresent auch einen doppler drin. :frowning:

ifPresent != isPresent

Aber intern zweimal eine if-Bedingung oder?

Ich würd das auch so übernehmen, aber es müsste Java 7 sein. :wink:

@Clayn : Bad Smells u. a. wegen break;, weiter usw. Das stößt sauer auf einigen.

Wieso wurde denn ein neuer Thread erstellt?

Weiter mit Beinahebugs: Wenn Java 8 nicht verwendet werden darf:

    @Override
    public String toString() {
        return table(tbody(each(rows, new Function<Reihe, DomContent>() {
            @Override
            public DomContent apply(Reihe t) {
                return tr(each(t.tds, new Function<WebElement, DomContent>() {
                    @Override
                    public DomContent apply(WebElement t) {
                        return td(t.getText());
                    }
                }), td(String.valueOf(t.tds.size())));
            }
        }))).renderFormatted();
    }

(nicht wundern, XXX und Reihe sind eigene Klassen - damit kann man sie nicht mit table oder tr verwechseln.)

Frage: Wie heißt die Klasse, deren toString() Methode hier zu sehen ist? (Legen Sie eine Vermutung nahe.)

Meine Vermutung:

Why

Die toString-Methode einer Klasse zu überschreiben und für so etwas zu missbrauchen halte ich nicht gerade für Best Practice.

Als nächstes möchte man dann vielleicht noch JSON als Ausgabe haben. Was macht man dann?

Okay, hab es geändert. Es geht immer noch um den Logger, den ich jetzt selber schreibe zu gedenke…

class Log {

    private static final ArrayList<ArrayList<Object[]>> LI = new ArrayList<>();

    static void log(String name, Object obj, boolean single) {
        Object[] oa = new Object[]{name, obj};
        if (single || !lastName().equals(name)) { // Tabellen sind immer Single
            ArrayList<Object[]> l = new ArrayList<>();
            l.add(oa);
            LI.add(l);
        } else {
            LI.get(LI.size() - 1).add(oa);
        }
    }

    static String lastName() {
        if (LI.isEmpty()) {
            return "empty";
        }
        return (String) LI.get(LI.size() - 1).get(0)[0];
    }

    static void printLog() {
        System.out.println(body(join(each(LI, new Function<ArrayList<Object[]>, DomContent>() {
            @Override
            public DomContent apply(ArrayList<Object[]> t) {
                if (t.get(0)[0].equals("Tabelle")) {
                    return table(tbody(each(((Tabelle) t.get(0)[1]).rows, new Function<Reihe, DomContent>() {
                        @Override
                        public DomContent apply(Reihe t) {
                            return tr(each(t.tds, new Function<WebElement, DomContent>() {
                                @Override
                                public DomContent apply(WebElement t) {
                                    return td(t.getText());
                                }
                            }), td(String.valueOf(t.tds.size())));
                        }
                    })));
                } else {
                    return join(each(t, new Function<Object[], DomContent>() {
                        @Override
                        public DomContent apply(Object[] t) {
                            return join(b((String) t[0]), t[1].toString());
                        }
                    }), br());
                }
            }
        })), hr()).renderFormatted());
    }
}

Dass der Compiler es auf „Richtigkeit“ überprüfen kann, ist schon ein kleines Wunder. Aber sieht jemand hierin ein Problem?

Das Problem ist, Log.printLog(); wird zum Schluss der Anwendung aufgerufen, .getText() basiert aber auf einem JNI-Aufruf, dh an der Stelle können wir uns nicht mehr auf den GC verlassen, und Teile des Objekts, auf dem .getText() aufgerufen wird, existiert zu dem Zeitpunkt nicht mehr. Oder anders, die Referenz wurde schon aufgeräumt.

Das zweite Problem ist die Liste von Listen von Feldern. Ich brauche eigentlich noch eine Klasse RegularEntryToAppend und eine Klasse TableEntryToAppend und eine Oberklasse (Abstract)EntryToAppend, aber das ist alles zu komplex, bzw ich will nicht so viele Klassen habn.

Hat jemand eine andere Idee?


Was wäre hieran richtig oder falsch??:

abstract class AbstractEntryToAppend {
    abstract String getName();
    abstract String getValue(); // Nur Nicht-Tabelle hat value....
    abstract String[][] getTable(); // Nur Tabelle hat table....
}

class RegularEntryToAppend extends AbstractEntryToAppend {
    String name, value;

    RegularEntryToAppend(String name, Object value) {
        this.name = name;
        this.value = value.toString();
    }

    @Override
    String getName() {
        return name;
    }

    @Override
    String[][] getTable() {
        return null;
    }

    @Override
    String getValue() {
        return value;
    }
}

class TableEntryToAppend extends AbstractEntryToAppend {
    String[][] table;

    public TableEntryToAppend(Tabelle tabelle) {
        //....
    }

    @Override
    String getName() {
        return "Tabelle";
    }

    @Override
    String[][] getTable() {
        return table;
    }

    @Override
    String getValue() {
        return null;
    }
}

Ich habe keine Zeit mir den ganzen Code anzusehen, denn der ist in keinem Falle Clean. Was ich aber einen unserer Azubis sofort fragen würde:

Warum ist das kein Interface, sondern eine abstrakte Klasse? Ich hoffe, dies beantwortet einen kleinen Teil der Frage „Was wäre hieran richtig oder falsch??“

Und jetzt??:

class Log {
    private static final ArrayList<ArrayList<AbstractEntryToAppend>> LI = new ArrayList<>();

    static void log(AbstractEntryToAppend aeta) {
        switch (aeta.getClass().getSimpleName()) {
            case "RegularEntryToAppend": {
                if (lastName().equals(aeta.getName())) {
                    LI.get(LI.size() - 1).add(aeta);
                } else {
                    ArrayList<AbstractEntryToAppend> l = new ArrayList<>();
                    l.add(aeta);
                    LI.add(l);
                }
                break;
            }
            case "TableEntryToAppend": {
                ArrayList<AbstractEntryToAppend> l = new ArrayList<>();
                l.add(aeta);
                LI.add(l);
                break;
            }
            case "ItemEntryToAppend": {
                if (lastName().equals(aeta.getName())) {
                    LI.get(LI.size() - 1).add(aeta);
                } else {
                    ArrayList<AbstractEntryToAppend> l = new ArrayList<>();
                    l.add(aeta);
                    LI.add(l);
                }
                break;
            }
            default:
                throw new AssertionError();
        }
    }

    static String lastName() {
        if (LI.isEmpty()) {
            return "empty";
        }
        return LI.get(LI.size() - 1).get(0).getName();
    }

    static void printLog() {
        System.out.println(body(each(LI, new Function<ArrayList<AbstractEntryToAppend>, DomContent>() {
            @Override
            public DomContent apply(ArrayList<AbstractEntryToAppend> t) {
                switch (t.get(0).getClass().getSimpleName()) {
                    case "RegularEntryToAppend": {
                        return div(b(t.get(0).getName()), text(": "), each(t, new Function<AbstractEntryToAppend, DomContent>() {
                            @Override
                            public DomContent apply(AbstractEntryToAppend t) {
                                return join(t.getValue(), br());
                            }
                        }), iff(t.size() > 1, br()));
                    }
                    case "TableEntryToAppend": {
                        return each(t, new Function<AbstractEntryToAppend, DomContent>() {
                            @Override
                            public DomContent apply(AbstractEntryToAppend t) {
                                return div(iff(!t.getTable().isEmpty(), div(b(t.getName()), text(": "), table(tbody(each(t.getTable(), new Function<ArrayList<String>, DomContent>() {
                                    @Override
                                    public DomContent apply(ArrayList<String> t) {
                                        return tr(each(t, new Function<String, DomContent>() {
                                            @Override
                                            public DomContent apply(String t) {
                                                return td(t);
                                            }
                                        }));
                                    }
                                }))), hr())));
                            }
                        });
                    }
                    case "ItemEntryToAppend": {
                        return div(b(t.get(0).getName()), text(": "), each(t, new Function<AbstractEntryToAppend, DomContent>() {
                            @Override
                            public DomContent apply(AbstractEntryToAppend t) {
                                return join(t.getItem().toString(), br());
                            }
                        }), iff(t.size() > 1, br()));
                    }
                    default:
                        throw new AssertionError();
                }
            }
        })).renderFormatted());
    }
}

abstract class AbstractEntryToAppend {
    abstract String getName();

    abstract String getValue(); // Nur Nicht-Tabelle hat value....

    abstract ArrayList<ArrayList<String>> getTable(); // Nur Tabelle hat table....

    abstract ArrayList<String> getItem(); // Nur Item hat item....
}

class RegularEntryToAppend extends AbstractEntryToAppend {
    String name, value;

    RegularEntryToAppend(String name, Object value) {
        this.name = name;
        this.value = value.toString();
    }

    @Override
    ArrayList<String> getItem() {
        return null;
    }

    @Override
    String getName() {
        return name;
    }

    @Override
    ArrayList<ArrayList<String>> getTable() {
        return null;
    }

    @Override
    String getValue() {
        return value;
    }
}

class TableEntryToAppend extends AbstractEntryToAppend {
    ArrayList<ArrayList<String>> table;

    public TableEntryToAppend(Tabelle tabelle) {
        table = new ArrayList<>();
        for (Reihe row : tabelle.rows) {
            ArrayList<String> l = new ArrayList<>();
            for (WebElement td : row.tds) {
                l.add(td.getText().replaceAll("[^\\w\\d ]+", ""));
            }
            table.add(l);
        }
        System.out.println(table);
    }

    @Override
    ArrayList<String> getItem() {
        return null;
    }

    @Override
    String getName() {
        return "Tabelle";
    }

    @Override
    ArrayList<ArrayList<String>> getTable() {
        return table;
    }

    @Override
    String getValue() {
        return null;
    }
}

class ItemEntryToAppend extends AbstractEntryToAppend {
    ArrayList<String> item;

    public ItemEntryToAppend(Item item) {
        this.item = new ArrayList<>(Arrays.asList(
                // Attribute sind geheim!!
        ));
    }

    @Override
    ArrayList<String> getItem() {
        return item;
    }

    @Override
    String getName() {
        return "Item";
    }

    @Override
    ArrayList<ArrayList<String>> getTable() {
        return null;
    }

    @Override
    String getValue() {
        return null;
    }
}

Das switch-case mag ich nicht so, wie vermeidet man es? Leider sind manche DIVs leer, das HTML ist aber immer valide.

Man schreibt doch abstrakte Klassen wirklich nicht so??

Polymorphismus :wink:
Das, was für die jeweiligen Klassen passieren soll, kannst du in den jeweiligen Klassen implementieren.

Und warum ist das ein Problem?

Deine abstrakte Klasse muss keine abstrakte Klasse sein - sie enthält ja nur abstrakte Methoden - man kann deshalb besser ein Interface draus machen.
Allerdings versteh ich die Richtung der Frage nicht…

Okay, denke, hab den Aufbau für den Logg: :wink:

Zusammenfassung
class Logg {

    Category<IItemEntry> cat;
}

class Category<A extends ISuperEntry> {

    void add(A a) {

    }
}

interface ISuperEntry {

    DomContent getValue();
}

interface IRegularEntry extends ISuperEntry {
}

interface IItemEntry extends ISuperEntry {
}

interface ITableEntry extends ISuperEntry {
}

interface IEmptyEntry extends IRegularEntry, IItemEntry, ITableEntry {

}

class EmptyEntry implements IEmptyEntry {

    @Override
    public DomContent getValue() {
        return null;
    }
}

class RegularEntry implements IRegularEntry {

    @Override
    public DomContent getValue() {
        return null;
    }
}

class ItemEntry implements IItemEntry {

    @Override
    public DomContent getValue() {
        return null;
    }
}

class TableEntry implements ITableEntry {

    @Override
    public DomContent getValue() {
        return null;
    }
}

Dafür kann man kein Interface schreiben, oder?

class Category<A extends ISuperEntry> {
    void add(A a) {
    }
}

Der generic type Angabe (<A extends ISuperEntry>) lässt sich auch nicht auf nur Interfaces festlegen oder? (Also ich könnte auch <ItemEntry> angeben, aber dann könnte ich EmptyEntry nicht mehr adden, man soll zwar immer gegen Interfaces programmieren, aber erzwingen lässt es sich nicht?)

Warum so kompliziert?

Category muss nur SuperEntry kennen und EmptyEntry kann doch direkt Sub-Klasse von SuperEntry sein, dann hast du dieses gesamte freut und quer nicht mehr.

Mit Java 8 schon, mit Java 7 nicht.

Die Typangabe ist doch ein Interface, gegen Interfaces zu programmieren ist damit erfüllt.

Erst mal schön, das du dir Zeit nimmst, und willkommen hier. :wink:
Zugegeben, mit maki verwechselt… (fast gleiches Symbol)

Das beschreibt es:

class Logg {
    public static void main(String[] args) {
        // Das Folgende soll erlaubt sein (ist es auch):
        Category<IItemEntry> catA = new Category<>();
        catA.add(new ItemEntry());
        catA.add(new ItemEntry());
        catA.add(new EmptyEntry());
        catA.add(new ItemEntry());

        // Das Folgende soll nicht erlaubt sein (ist es auch nicht):
        catA.add(new RegularEntry()); // incompatible types: RegularEntry cannot be converted to IItemEntry
        catA.add(new TableEntry()); // incompatible types: TableEntry cannot be converted to IItemEntry

        // Das Folgende soll erlaubt sein (ist es auch):
        Category<ITableEntry> catB = new Category<>();
        catB.add(new TableEntry());
        catB.add(new EmptyEntry());
        catB.add(new TableEntry());
        catB.add(new EmptyEntry());

        // Das Folgende soll nicht erlaubt sein (ist es aber):
        Category<ISuperEntry> catC = new Category<>(); // erlaubt alles
        Category<IEmptyEntry> catD = new Category<>(); // erlaubt nur Empty
        Category<ItemEntry> catE = new Category<>(); // erlaubt nicht Empty; erlaubt nur ItemEntry
        catD.add(new ItemEntry()); // (incompatible types: ItemEntry cannot be converted to IEmptyEntry)
        catE.add(new EmptyEntry()); // (incompatible types: EmptyEntry cannot be converted to ItemEntry)
    }
}

Wahrscheinlich lässt es sich nicht vereinfachen, da „mehrfachvererbung“ nur bei Interfaces.

Edit: In der Begründung der letzten beiden Zeilen war ein Dreher.

Wie lös ich jetzt das Problem, ohne Typfallunterscheidung, übergebene ISuperEntry s in IItemEntry, ITableEntry oder IRegularEntry -Listen richtig einzuordnen mit Hilfe Polymorphismus?

Es gibt drei Listen mit den jeweiligen Interface -Typen.


Und noch mal die andere Frage… Man kann nicht verhindern, dass eine Variable Typs Collection mit LinkedList anstatt List parametrisiert wird, oder?


Edit (und Guten Morgen): Mir leuchten Vorteile des Polymorphismus nicht ein. Ok, ich kann mehrere Typen in eine Liste stecken. Wenn ich aber hinterher eine (Typ)Fallunterscheidung brauche, dann ist das doch nicht im Sinne des…

Wie macht das eigentlich ein normaler Logger? Es gibt 4 Methoden, info error warning debug, denen Text übergeben werden kann, und eine Konfiguration der Ausgabe. Mehr nicht.

Gedacht war ja mein Logger, dass Log.log(new ItemEntry(…)), Log.log(new TableEntry()) usw. usf. aufgerufen werden kann. Why dann nicht einfach Log.logItem(…), Log.logTable(…) usw.

I-was hab ich nicht verstanden.

Die Log-Methode für ein Item sieht jetzt so (ich wechselte doch zu Java-8):

    static void logItem(float time, Item i) {
        Field[] fields = i.getClass().getDeclaredFields();
        String[] vals = new String[fields.length];
        try {
            for (int j = 0; j < fields.length; j++) {
                if (fields[j].getName().equals("clickable")) {
                    vals[j] = ((WebElement) (fields[j].get(i))).getText();
                } else {
                    vals[j] = fields[j].get(i).toString();
                }
            }
        } catch (IllegalArgumentException | IllegalAccessException ex) {
        }
        System.out.println(div(b(time + " Item "), table().withStyle("display: inline;").with(tbody(tr(each(Arrays.asList(vals), val -> td(val.replaceAll("[^\\w\\d ]+", ""))))))).renderFormatted());
    }

Ein Item hat viele Attribute, und ich hatte keine Lust, alle in der toString aufzulisten. Würdet ihr das auch so machen?

Zudem sieht eine Table in einer Zeile (inline) etwas komisch aus, aber das ist nicht so wichtig.

Das hättest du erwähnen sollen, das kommt oben nicht vor :wink:

Man kann zB den Log an die jeweilige Klasse übergeben, und diese sucht sich die entsprechende Liste raus. Führt natürlich zu hoher Kopplung.

Nein, kann man nicht.

Die Fallunterscheidung brauchst du eben nicht mehr - das unterschiedliche Verhalten steckt in den unterschiedlichen Klassen :wink:

Bei einem „normalem“ Logger gibts das Problem nicht - es gibt keine Unterschiedlichen Klassen für unterschiedliche Level, die dann unterschiedliche Dinge bewirken sollen.

Das kann man durchaus zu umsetzten, kommt halt immer drauf an wie das Interface nachher aussehen soll…

Nein, Reflection würde ich nicht dafür missbrauchen.

Wenn das zu viele Attribute sind, änder dein Design entsprechend.

Hab gerade ein Bottelneck (Engpass) (von 2 Minuten), denn ich eigentlich vermeiden oder parallelisieren (damit’s nur 30 Sek. sind) wollte, und weiß nicht genau, woran’s liegt:

    private static final LinkedHashMap<String, Sys> syss = new LinkedHashMap<>();

    //...

        ArrayList<Sys[]> als = new ArrayList<>();
        for (Sys sys1 : syss.values()) {
            for (Sys sys2 : syss.values()) {
                float d = (float) Math.sqrt((sys1.x - sys2.x) * (sys1.x - sys2.x) + (sys1.y - sys2.y) * (sys1.y - sys2.y) + (sys1.z - sys2.z) * (sys1.z - sys2.z));
                if (d >= A_MAGIC_VALUE_ONE && d <= A_MAGIC_VALUE_TWO) {
                    als.add(new Sys[]{sys1, sys2});
                }
            }
        }
        System.out.println("als.size() = " + als.size());

Ich gebe euch ein paar Eckdaten dazu:

syscount = 36565
stacount = 49589
buycount = 37017
sellcount = 37017
als.size() = 199910

Es gibt ~ 36t Systeme, ~ 50t Stationen (buy, sell nicht wichtig).

Fragen:
Ist LinkedHashMap langsam?
Ist Math.sqrt() langsam?
Ist 36565^2 (die zwei Schleifen) langsam?
Ist new Sys[] mit zwei Refs langsam?

Ich hab den Verdacht :confused: , dass es an der Berechnung liegt — 1,3 Mrd. etwas berechnen, sollte nicht das Problem sein - oder?

Ich weiss nicht was daran schnell oder langsam ist, aber du machst zumindest unnötige Rechenschritte.

Math.sqrt sieht zumindest mal aufwendig aus und ist zudem auch nicht wirklich gebraucht

Mal ein Beispiel

float d = Math.sqrt(9);
if(2 <= d && d <= 5) { //Bla}

Ist eigentlich das selbe wie das da:

float d = 9;
if(4 <= d && d <= 25) { //Bla}

Wenn du deine MAGIC_VALUES quadrierst, am besten ausserhalb der Schleife, dann brauchst du beim Vergleich gegen diese MAGIC_VALUES auch keine Wurzel mehr ziehen.
So sollte also das selbe rauskommen bei weniger Zeit.

Um aber richtig dahinterzukommen, was da genau schief läuft, würde ein Profiler helfen.

Mal schauen wie lange es geht, bis der Beitrag gelöscht wird :wink: der so eigentlich ein eigenes Thema verdient hätte, weil nichts mit dem Ursprünglichen zu tun.

1 „Gefällt mir“