Refactoring, Netbeans

CyborgBeta · 18. Juli 2015 um 09:24

Hallo Leute,

es geht um die folgende Klasse:

die Implementierung müsst ihr gar nicht kennen,

sind die Methoden/Funktionen richtig benannt?,
enthalten die folgenden Funktionen so genannte Redundanzen?:

        if (ba == null || !buffer.get(index).isEmpty() && equalsArray(buffer.get(index).getLast(), ba)) {
            return;
        }
        buffer.get(index).add(ba);
    }```

```    private static boolean equalsArray(byte[] b1, byte[] b2) {
        if (b1 == null || b2 == null) {
            return true;
        }
        return Arrays.equals(b1, b2);
    }```

ich hab mir jetzt gedacht, null sei in der Collection nicht.

Grüße CB!

Firephoenix · 19. Juli 2015 um 01:03

Schau mal von außen drauf. Macht der Code für dich Sinn?

equalsArray(null, new byte[] {1,2,3}) = true

        List<LinkedList<byte[]>> outer = new ArrayList<>();
		LinkedList<byte[]> inner = new LinkedList<>();
		inner.add(new byte[] { 1, 2, 3 });
		outer.add(inner);
		System.out.println(outer.get(0).size()); //-> 1
		insertArray(outer, new byte[] { 1, 2, 3 }, 0);
		System.out.println(outer.get(0).size()); //-> 1???```

```		List<LinkedList<byte[]>> outer = new ArrayList<>();
		LinkedList<byte[]> inner = new LinkedList<>();
		inner.add(new byte[] { 1, 2, 3 });
		// outer.add(inner);
		insertArray(outer, new byte[] { 1, 2, 3 }, 0); //IOOBException```

CyborgBeta · 19. Juli 2015 um 05:23

            return;
        }```
wäre das besser?

Buffer kann noch nichts enthalten, dann soll eingefügt werden, wenn Neues != null ist,
enthält Buffer bereits was, dann soll eingefügt werden, wenn sich das zuletzt Eingefügte von dem Neuen unterscheidet,

null soll im Buffer nicht erlaubt sein, das ist dann wieder beim Speichern schwierig.

Verstehst du, aws ich meine?

Marco13 · 19. Juli 2015 um 07:31

OK, Firephoenix hat sich aufgerafft. Naja.

Abgesehen von solchen “Java Programming 101”-Fehlern, wie dass ArrayList<LinkedList<byte[]>> buffer NIE (NIE) in einer Methodensignatur vorkommen sollte, ist für die Beantwortung der Frage, ob die Methoden richtig benannt sind, durchaus relevant, was die Methoden machen.

Anhand der Auflistung der Methodennamen kann man bisher nur sagen, dass es STARK danach aussieht, als würden dort Dinge vermischt, die nicht vermischt werden sollten. Einmal geht es um irgendwelche Buffers (und Arrays), dann um URLs, und dann steht da noch so ein unmotiviertes “sleep” dabei … Hm.

““Profi”-Tipp”: Schreib’ JavaDoc-Kommentare. Wenn du bei einer Klasse ansetzt, einen Kommentar zu schreiben

/** 
 * This class is a ... well, I'm not sure what it is. 
 * It does some things with buffers, but also does
 * some things with URLs, because I did not know
 * where else I should put these methods. I also
 * inserted the sleep method here, because when
 * I use this class, I also want to sleep sometimes
 */

ist das ein deutliches Zeichen dafür, dass etwas nicht stimmt.

Genauso bei den Methoden. Wenn du die Kommentare zu clearBuffer und fullClearBuffer schreibst, und merkst, dass sie fast gleich sind (oder du Schwierigkeiten hast, in einem, KURZEN Satz klarzumachen, was die beiden Methoden machen und wo der Unterschied ist), ist das ein Warnsignal.

Ansonsten hier noch eines der ganz, ganz wenigen Videos, bei denen ich uneingeschränkt eine Anseh-Empfehlung abgebe: How to Design a Good API & Why it Matters

CyborgBeta · 19. Juli 2015 um 10:24

Danke für eure beiträge.

Jetzt sieht es so aus:

        if (ba == null) {
            return;
        }
        if (buffer.isEmpty()) {
            buffer.get(index).add(ba);
        }
        if (equalsArray(buffer.get(index).getLast(), ba)) {
            buffer.get(index).add(ba);
        }
    }


    private static boolean equalsArray(byte[] b1, byte[] b2) {
        if (b1 == null || b2 == null) { // unnoetig
            return true;
        }
        return Arrays.equals(b1, b2);
    }```

Die erste Methode enthält so genannte Redundanzen, Redundanzen sind schlecht, das Universum wird dadurch instabil.^^ In der zweiten ist sicher, wenn nur insertArray sie aufruft, wird nicht null eingefügt.

Jetzt weg von "How to name methods java?" sieht die "sleep"-Methode so aus (Hilfe von mogel erhalten):

```    private static boolean sleep(long start, long stop) throws InterruptedException {
        long sleep = 123 * 1000;
        long sctm = System.currentTimeMillis();
        long millis = sleep - sctm % start % sleep;
        System.out.println("Millis = " + millis);
        if (sctm + millis > stop) {
            return false;
        }
        System.gc();
        Thread.sleep(millis);
        return true;
    }```

Bestimmt auch verbessert werden kann.

Grüße CB!

Edit: Siehste, ist schon ein Logik-Fehler drin, jetzt könnte ba zweimal eingefügt werden, das ist mir gerade noch aufgefallen, bevor es online ging.

CyborgBeta · 20. Juli 2015 um 05:57

Verbessert, richtige Methode und Name:

        for (int i = 0; i < urlStrings.length; i++) {
            buffer.add(new LinkedList<byte[]>());
        }


    private static void insertArray(List<Deque<byte[]>> buffer, byte[] ba, int index) {
        if (!equalsArray(buffer.get(index).peekLast(), ba)) {
            buffer.get(index).add(ba);
        }
    }

    private static boolean equalsArray(byte[] b1, byte[] b2) {
        if (b1 == null || b2 == null) { // FIXME
            return true;
        }
        return Arrays.equals(b1, b2);
    }```

Eine Liste bietet wahlfreien Zugriff, eine Deque ist wie ein Queue + Stack (schnell).

SlaterB · 20. Juli 2015 um 06:08

nur damit hier niemand was falsches abschreibt:
bei der equalsArray-Methode hat der Hinweis

[quote=Firephoenix]Schau mal von außen drauf. Macht der Code für dich Sinn?

equalsArray(null, new byte[] {1,2,3}) = true[/quote]

bisher zu
if (b1 == null || b2 == null) { // unnoetig
und später zu
if (b1 == null || b2 == null) { // FIXME
geführt?

na, die eine Weisheit sei hingeschrieben,
jede equals-Methode von zwei Dingen a und b kann man mit

if (a == null || b == null) return false; // maximal einer null
// hier nix mehr null

beginnen

so steht es freilich auch in Arrays.equals(), weglassen also eine gute Option,
aber um gezielt das einzig falsche zu machen muss es natürlich stehen bleiben, bestens…

CyborgBeta · 24. Juli 2015 um 11:36

@SlaterB : Dieses Thema ist wohl durch, ich hab nicht den Anforderungen entsprochen, aber dennoch nachreichen:

        for (int i = 0; i < urlStrings.length; i++) {
            buffer.add(new LinkedList<byte[]>());
        }

    private static void insertArray(List<Deque<byte[]>> buffer, byte[] ba, int index) {
        if (validArray(buffer.get(index).peekLast(), ba)) {
            buffer.get(index).add(ba);
        }
    }

    private static boolean validArray(byte[] b1, byte[] b2) { // das sieht jetzt wirklich abenteuerlich aus
        if (b1 == null) {
            return b2 != null;
        } else {
            if (b2 == null) {
                return false;
            } else {
                return !Arrays.equals(b1, b2);
            }
        }
    }

    private static void clearBuffer(List<Deque<byte[]>> buffer) throws IOException {
        for (int i = 0; i < buffer.size(); i++) {
            if (buffer.get(i).size() >= 3) {
                while (buffer.get(i).size() > 1) {
                    writeArray(buffer.get(i).remove(), i);
                }
            }
        }
    }

    private static void fullClearBuffer(List<Deque<byte[]>> buffer) throws IOException {
        for (int i = 0; i < buffer.size(); i++) {
            while (!buffer.get(i).isEmpty()) {
                writeArray(buffer.get(i).remove(), i);
            }
        }
    }

    private static boolean sleep(long start, long stop) { // true , wenn geschlafen werden kann
        long sleep = 111 * 1000;
        long sctm = System.currentTimeMillis();
        long millis = sleep - sctm % start % sleep;
        System.out.println("Millis = " + millis);
        if (sctm + millis > stop) {
            return false;
        }
        System.gc();
        try {
            Thread.sleep(millis);
        } catch (InterruptedException ie) {
            return false;
        }
        return true;
    }```

(Danke für dDeine Antwort)

SlaterB · 24. Juli 2015 um 13:13

warum auch nicht ohne Not so einen Mist bauen obwohl bessere Lösung schon gepostet/ allgemein bekannt (auch in Arrays.equals)

bevor das wer liest müsste man es eigentlich löschen, aber naja

Bleiglanz · 24. Juli 2015 um 22:21

[quote=SlaterB]warum auch nicht ohne Not so einen Mist bauen obwohl bessere Lösung schon gepostet/ allgemein bekannt (auch in Arrays.equals)

bevor das wer liest müsste man es eigentlich löschen, aber naja[/quote]

Das fällt auch wirklich in eine eigene Kategorie, CB-Style, not even wrong

private static boolean validArray(byte[] b1, byte[] b2) { // das sieht jetzt wirklich abenteuerlich aus
        if (b1 == null) {
            return b2 != null; // wenn b1==null und b2 != null wird true zurückgegeben: sehr abenteuerlich
        } else {
            if (b2 == null) {
                return false;
            } else {
                return !Arrays.equals(b1, b2); // die Methode heisst validArray? Zurückgegeben wird !equals?
            }
        }
    }

man müsste halt mal 5 Minuten verwenden und was nachschlagen

public static boolean equals(Object[] a, Object[] a2) {
    if (a==a2)
        return true; // ja sapperlot, Array.equals testet sowie so ob das null ist....
    if (a==null || a2==null)
        return false;

    int length = a.length;
    if (a2.length != length)
        return false;

    for (int i=0; i<length; i++) {
        Object o1 = a**;
        Object o2 = a2**;
        if (!(o1==null ? o2==null : o1.equals(o2)))
            return false;
    }

    return true;
}```