Programmierstil/Best practices: Bei MinMax-ähnlichen Methoden Doppelung vermeiden


#1

Rundreise unheuristisch:

        @Override
        public void run() {
            a(0, 0, new ArrayDeque<>());
            prozent = 0;
            b(0, 0, new ArrayDeque<>());
        }

        void a(int index, int syss1index, ArrayDeque<Pro2> list1) {
            if (index == routelen) {
                Sys s1 = syss2.get(list1.getLast().sysid2);
                Sys s2 = syss2.get(list1.getFirst().sysid1);
                float d = (s1.x - s2.x) * (s1.x - s2.x) + (s1.y - s2.y) * (s1.y - s2.y) + (s1.z - s2.z) * (s1.z - s2.z);
                if (d <= maxdis) {
                    ArrayList<Pro2> pro2s = getPro2s(s1, s2, maxbuy, minpro);
                    if (pro2s != null) {
                        for (Pro2 pro2 : pro2s) {
                            if (list1.isEmpty() || list1.getLast().staid2 == pro2.staid1 && list1.getFirst().staid1 == pro2.staid2) {
                                list1.add(pro2);
                                list2.add(new ArrayList<>(list1));
                                list1.removeLast();
                            }
                        }
                    }
                }
                return;
            }
            if (index == 0) {
                index++;
                for (int j = from; j < to; j++) {
                    a(index, j, new ArrayDeque<>());

                    if ((j - from) * 100 / (to - from) > prozent) {
                        prozent = (j - from) * 100 / (to - from);
                        println(this + " , pro = " + prozent);
                    }
                }
            } else {
                index++;
                Sys s1 = syss1.get(syss1index);
                for (int j = syss1index + 1; j < syss1.size(); j++) {
                    Sys s2 = syss1.get(j);
                    float d = (s1.x - s2.x) * (s1.x - s2.x) + (s1.y - s2.y) * (s1.y - s2.y) + (s1.z - s2.z) * (s1.z - s2.z);
                    if (d <= maxdis) {
                        ArrayList<Pro2> pro2s = getPro2s(s1, s2, maxbuy, minpro);
                        if (pro2s != null) {
                            for (Pro2 pro2 : pro2s) {
                                if (list1.isEmpty() || list1.getLast().staid2 == pro2.staid1) {
                                    list1.add(pro2);
                                    a(index, j, list1);
                                    list1.removeLast();
                                }
                            }
                        }
                    }
                }
            }
        }

        void b(int index, int i, ArrayDeque<Pro2> list1) {
            if (index == routelen) {
                Sys s1 = syss2.get(list1.getLast().sysid1);
                Sys s2 = syss2.get(list1.getFirst().sysid2);
                float d = (s1.x - s2.x) * (s1.x - s2.x) + (s1.y - s2.y) * (s1.y - s2.y) + (s1.z - s2.z) * (s1.z - s2.z);
                if (d <= maxdis) {
                    ArrayList<Pro2> pro2s = getPro2s(s2, s1, maxbuy, minpro);
                    if (pro2s != null) {
                        for (Pro2 pro2 : pro2s) {
                            if (list1.isEmpty() || list1.getLast().staid1 == pro2.staid2 && list1.getFirst().staid2 == pro2.staid1) {
                                list1.add(pro2);
                                list2.add(new ArrayList<>(list1));
                                list1.removeLast();
                            }
                        }
                    }
                }
                return;
            }
            if (index == 0) {
                index++;
                for (int j = from; j < to; j++) {
                    b(index, j, new ArrayDeque<>());

                    if ((j - from) * 100 / (to - from) > prozent) {
                        prozent = (j - from) * 100 / (to - from);
                        println(this + " , pro = " + prozent);
                    }
                }
            } else {
                index++;
                Sys s1 = syss1.get(i);
                for (int j = i + 1; j < syss1.size(); j++) {
                    Sys s2 = syss1.get(j);
                    float d = (s1.x - s2.x) * (s1.x - s2.x) + (s1.y - s2.y) * (s1.y - s2.y) + (s1.z - s2.z) * (s1.z - s2.z);
                    if (d <= maxdis) {
                        ArrayList<Pro2> pro2s = getPro2s(s2, s1, maxbuy, minpro);
                        if (pro2s != null) {
                            for (Pro2 pro2 : pro2s) {
                                if (list1.isEmpty() || list1.getLast().staid1 == pro2.staid2) {
                                    list1.add(pro2);
                                    b(index, j, list1);
                                    list1.removeLast();
                                }
                            }
                        }
                    }
                }
            }
        }

syss1 “index iterable”
syss2 “SysID pullable”
Pro2 way from Sys a/Sta a to Sys b/Sta b inclusive costs.
if (index == 0) { Der beginn der rekursiven Methode
} else { Zwischenschritt(e)
if (index == routelen) { ende der rekursiven Methode
a und b rekursive Methoden, nicht wechselseitiger Aufruf
list1 “ordered temporary results (“one cyclic route”)”
list2 “ordered results”

Wie kann man das kürzen und auch beispielsweise if (list1.isEmpty() || list1.getLast().staid2 == pro2.staid1) { und if (list1.isEmpty() || list1.getLast().staid2 == pro2.staid1 && list1.getFirst().staid1 == pro2.staid2) { kürzen?

Mir ist das zu unübersichtlich und auch doppelter Code, schwierig nachzuvollziehen und zu erweitern. :frowning:

void b(int index, int i das sollte eigentlich heißen void b(int index, int syss1index und j müsste eigentlich syss1index2 heißen. :frowning:


#2

Das Ganze ist wirklich schlecht lesbar und nachvollziehbar. Nach 4 Wochen weißt du nicht mehr, was du da eigentlich warum mal geschrieben hast.
Man könnte private Methoden einführen, die den doppelten Code auflösen. Also das Ganze ein wenig in Teilschritte gliedern. Das fördert Lesbarkeit und Wiederverwendbarkeit. Ein paar sprechende Variablen tun dann noch ihr Übriges.


#3

Mir auch. Den Code lese ich nicht, weswegen ich nur auf deine konkreten Fragen eingehen werde:

Indem man die Prüfung in eigene Methode auslagert.Das ich keine Ahnung hab, was dein Code tut, nehm ich ein konstruiertes Beispiel:

if(userInput != null && userInput.length > 2) {}

könnte man auslagern in eine validierungsmethode

if(validateUserInput(userInput)) {}

Beim schnellen drüber lesen weißt du was passiert (eine Validierung) und musst nicht erst versuchen Code zu verstehen.

Keine Ahnung was du damit sagen willst. Aber: Nein. Was tut denn die Methode b? was ist j? Vergib richtige Namen, die dir auch helfen. Wenn man erst in die Methode reinschauen muss und den kompletten Code zu verstehen, dann ist das einfach schlechter Code. Vor allem wenn es weitergeht mit schlecht benamten Variablen. Letztendlich ist der Code auch noch undokumentiert.

Und was mir doch aufgefallen ist: Verändere keine Parameter! Du änderst den Parameter index. Dafür hab ich so manchen Kollegen aus meiner letzten Firma verflucht. Da hatten die Helden auch so lange Methoden geschrieben wie du oben hast (und länger). Hinzu kam, dass Parameter komplett verändert wurden und ich mir nicht mehr sicher sein konnte, was ich an entsprechender Stelle überhaupt bekomme. Das ist ein extrem hohes Risiko einen Bug zu produzieren!


#4

Bin noch nicht ganz sicher, wie ich es aus den zwei rekursiven Methoden nur eine mach.

@ TS : Rekursive Methoden sind über sich ändernde Parameter definiert. Das ergibt keinen sinn, wenn du dich über deinen Kollegen ärgerst, schreib das in den Frust-Thread, und lass deine Aversionen nicht an mir aus.


#5

Das war ein Beispiel, warum man sowas nicht machen soll. Und doch ergibt es Sinnn. Wenn du mit anderen Werten arbeiten willst, dann nutze variablen. Aber siehe Parameter immer als final an.

Beispiel:

public void processArticle(int articleNumber) {
    // a lot of lines
    if(isPromotion(articleNumber)) {
        articleNumber = getPromotionParentArticle(articleNumber);

        runStatisticsOnPromotion(articleNumber);
    }
    // a lot of lines
    
    if(!isInStock(articleNumber)) {
        throw new ArticleNotInStockException(articleNumber);
    }
}

Wie du siehst, isInStock verlässt sich auf eine Artikelnummer die mittlerweile zu einem übergeordneten Artikel verändert worden ist. Das könnte zu einem Fehler führen, der keine ist. Das problem wäre keins, wenn der Parameter final ist oder zumindest effectiv final. Dann musst du eine Variable verwenden. Also:

public void processArticle(final int articleNumber) {
    // a lot of lines
    if(isPromotion(articleNumber)) {
        int promoArticleNumber = getPromotionParentArticle(articleNumber);

        runStatisticsOnPromotion(promoArticleNumber);
    }
    // a lot of lines

    if(!isInStock(articleNumber)) {
        throw new ArticleNotInStockException(articleNumber);
    }
}

Bug behoben und es kann keine falsche ArticleNotInStockException fliegen.


#6

Eigentlich würde ich sowas per PN klären. Ganz sachlich. Ohne viel Aufsehen. Damit mir nicht “Mobbing” oder so vorgeworfen wird. Aber bei dir mache ich da mal eine Ausnahme: Halte dich zurück!. Der Hinweis war berechtigt (und mit einer Anekdote begründet). Wenn du den Hinweis nicht verstehst, oder nicht nachvollziehen kannst, bin ich nicht sicher, ob das mit mangelnder Programmiererfahrung zu tun hat, oder ob es nicht tiefer liegende Ursachen hat, die vielleicht auch die Ursache für mangelnde Programmierfähigkeiten sind. Man könnte noch wohwollend sagen, dass es ja gerade darum ging, Verbesserungsvorschläge einzuholen. Aber erstens ist der Code wirklich unter aller Sau, zweitens nimmst du Ratschläge (offenbar) nicht so gerne an, und drittens beschäftigst du dich eigentlich schon lange genug mit Java und Programmierung (und hast schon oft genug das gleiche gesagt bekommen) dass man doch ein Mindestmaß an ““Codequalität”” erwarten können sollte, gerade wenn Leute das ganze reviewen sollen.

Ich mache gerne Code-Reviews. Entweder, um Konzepte und “Best Practices” zu erklären, oder (wennn es da nichts mehr zu erklären gibt, weil der Code schon “gut” ist), dann doch noch um an irgendwelchen Kleinigkeiten rumzumeckern. Wer das auch mag, kann sich auf https://codereview.stackexchange.com/ austoben. Aber dein Code ist so derart unglaublich haarsträubender Murks, dass ich schlicht sagen muss: Das ist weeeeit unter meinem Niveau.


#7

@Marco13 : Auch das von dir ist Nonsense… Er hätte gleich sagen können, dass ihm das microoptimierte

index++;

NICHT gefällt. Dann hätte jeder Bescheid gewusst. Nein Aber stattdessen bringt er die Metapher (Anekdote?) mit Kollegen.

Und Nö, ich werde mich nicht beruhigen. Mir kommt das vor wie irgendein sinnloses Politikergeschwafel, was ihr beide hier ablasst.


#8

“Hast du gehört, auf der A9 gab es einen Geisterfahrer!” “Einen??? Es waren hunderte!”

Erst einmal finde ich es gut, dass du überhaupt um Verbesserungsvorschläge bittest. Aber wenn von mehreren Seiten übereinstimmend die Einschätzung kommt, dass der Code unter aller Sau ist (was er leider ist), solltest du zumindest in Erwägung ziehen, dass da etwas dran ist. Und wenn du Kritik an deinem Code nicht von Kritik an deiner Person trennen kannst, du sofort alles als Angriff empfindest, hast du leider den falschen Berufsweg eingeschlagen. Dass du mit der Kritik nicht viel anfangen kannst, muss auch nicht daran liegen, dass diese schlecht ist*), sondern kann auch damit zusammenhängen, dass du eben noch nicht so weit bist, bestimmte abstrakte Kritik zu verstehen. Und an diesem Punkt war jeder von uns hier, jeder hat einmal so etwas wie “warum regen die alle sich wegen des Singleton so auf und kommen mit diesem komplizierten Factory oder DI Krams an?” gedacht. Das Verständnis kommt nicht von selbst, auch nicht allein vom Programmieren, man muss aktiv und vor allem konstruktiv davon arbeiten. Also, tief durchatmen, die Beiträge noch man in Ruhe durchlesen, keine böse Absicht unterstellen und nachfragen, wenn etwas unklar ist. Du hast selbst gefragt, also mache dir bitte auch die Mühe, die Antworten zu verstehen, auch wenn sie dir nicht gefallen.

*) So verstösst z.B. das Ändern von Methodenparametern in meiner Firma gegen best practices und würde durch kein Code Review gehen, der Verbesserungsvorschlag also aus meiner Sicht völlig korrekt.


#9

Ich hab mir bei meinem ersten Beitrag hier den Satz “das hab ich dir aber schon 1000x gesagt” echt hart verkneifen müssen. Da war ich einfach tatsächlich so blöd und dachte, du wärst vielleicht mal etwas reifer geworden und wollte dir eine möglichst unvoreingenommene Antwort geben, wie sie jeder andere von mir auch bekommen hätte. Das du daraus ein persönliches Drama gestaltest (WIE SO OFT!) habe ich aus Respekt zu deinem Thread selbst nicht persönlich genommen und gedacht, der Hinweis das es ein Beispiel ist reicht aus um es aufzuklären. Da du meinen Punkt nicht verstanden hast, hab ich dir sogar noch ein Codebeispiel geliefert - was deutlich zeigt, was ich meine.

Ich weiß nicht was falsch bei dir läuft, dass du alles IMMER UND IMMER WIEDER auf eine persönliche Ebene ziehst. Und ehrlich gesagt: es ist mir auch herzlich egal. Wir sind keine psychologen-Forum das dich analysiert oder deine Eltern, die dich erziehen. Mit dieser Art sabotierst du hier gerade deinen eigenen Thread und du bist kurz davor, dass ich den dicht mache — und in nächster Zeit dann auch jeden weiteren wo du nach Coderview fragst. Warum? Weil ich davon ausgehen muss, dass du die Leute hier trollst. Du fragst nach Vorschlägen, maulst die Leute an, wenn du welche bekommst und denkst im Traum nicht dran etwas davon umzusetzen. Dein Code schaut heute noch genauso schlimm aus wie vor Monaten. Dementsprechend muss ich von ausgehen, dass du nichts verbessern willst. Du willst nur die Zeit der Leute hier stehlen und provozieren.


#10

Nagut, wenn ich jetzt sage, ich hab verstanden, klinge das auch etwas “schräg”.

Ich hab nicht behauptet, dass der Code ganz toll wäre. Das ist halt privat geschrieben.

Ich hab nur bemerkt, dass ich zwei sehr ähnliche Methoden mit fast doppelten Code hab, und ich weiß NICHT gerade, wie das zu umgehen wäre.

Wirklich, kein Getrolle, einfach eine Code-Review frage bei privaten Code.


Wenn es wirklich “Unter aller Sau” ist, dann muss das Thema ggf gelöscht werden Punkt


#11

^this!!!

Man sieht sich leider oft genötigt, das zu betonen. Ich kritisiere Code, und nicht die Person, die ihn geschrieben hat. (Diese Kritik kann sich auch auf Code beziehen, den ich selbst geschrieben habe. Aber das ständig (schon während des Schreibens!) zu tun gehört zum Codeschreiben dazu!)

Ob da irgendein Zusammenhang besteht, muss dann schon die Person entscheiden. Wenn derjenige sagt: “Ja, ich weiß, dass das kagge ist, aber das ist eben so schlecht wegen [irgendein plausibler Grund - ‘Zeitmangel’ geht praktisch immer]”, dann ist das so. Wenn derjenige sich genötigt sieht, zu sagen: “Oh, ja, stimmt, das ist kagge, aber ich hab’s einfach nicht besser hingekriegt”, dann ist durchaus nachvollziehbar, dass man sich irgendwie unwohl fühlt: Niemand wird gerne kritisiert, und wenn etwas am Code (!) kritisiert wird, und man erkennt, dass sich in diesen Kritikpunkten irgendeine Art von eigener “Inkompetenz” widerspiegelt, fühlt man sich natürlich nicht wohl.

Für mich persönlich bedeutet das unter anderem (auch, wenn der Zusammenhang da im ersten Moment weit hergeholt erscheinen mag), dass ich versuche, zu vermeiden, dass “low-level-Kritik” angebracht werden kann. Das geht (bei mir) sehr weit - leider schon ins zwanghaft-pedantische :pensive: Aber das führt so gesehen zum nächsten Punkt:

Und “privat geschrieben” ist ein Argument … wofür?

Alles in meinen GitHub Repos ist “privat geschrieben”. Man kann doch zumindest versuchen, eine Baseline zu setzen. Wenn man diese Baseline gesetzt hat, kann man sich bei einem Code-Review nämlich auf die interessanten Sachen beziehen: Konzepte, Algorithmen, Datenstrukturen, Schnittstellen, Schnittstellen, Schnittstellen (!), und auch “Stil”, worüber man beliebig lange streiten kann.


#12

Hm, ihr hattet alle recht. Ich kann das heute nur noch schwer nachvollziehen und es waren auch Fehler drin. index++; sollte so nicht geschrieben werden und die Variablenbezeichnungen sind teilweise furchtbar (falsch).

Außerdem hatte ich mich wohl leider im Ton vergriffen. Entschuldigung dafür. Tomate_Salat, tut mir auch echt Leid, dass ich so unfreundlich war.

(Ich hätte die “Präambel” auch besser schreiben müssen. Bzw. es gab gar keine)


Mit funktionaler Programmierung kann man auch nicht vermeiden, dass es zwei Methoden gibt oder?