[Java] Verkapte GoTo?

[MichielPH]

Ben benieuwd of volgend stuk wordt gezien als misbaksel en zo ja, hoe ik dit het beste kan verbeteren.

De code is onderdeel van een game; het doel is om met vaste interval zogenoemde weeples te creëren. Dit gebeurt op basis van een mParameters, wat een HashMap<String, Object> is. Deze bevat weeple_type_i en weeple_count_i, met i als iterator.beginnende bij 0 en een onbekende max.

Stel dat de HashMap dit bevat:

code:

weeple_type_0: "Orange"
weeple_count_0: 2
weeple_type_1: null
weeple_count_1: 1
weeple_type_2: "Black"
weeple_count_2: 2
repeat: 1

Dan is dit het resultaat: Orange, Orange, null, Black, Black, Orange, Orange, null, Black, Black

Om dit te bereiken schreef ik dit:

Java:

public class Entrance extends AbstractElement {

    int mWeepleCount;
    int mRow, mRowWeepleCount;
    
    int mRepeatCount, mRepeatMax;
    
    ElementParameter mParameters;
    
    public Entrance(ElementParameter parameters) {
        mParameters = paramters;
        mRepeatMax = parameters.getInt(ElementParameter.REPEAT, 1);
    }
    
    public void onTick(float delta) {
         mGameTime += delta;
         
        if (mGameTime *  mFrequency > mWeepleCount) {
            createWeeple();
        }
        
    }
    
    public void createWeeple() {
        WeepleType weepleType = null;
        while (true) {
            if (mRepeatCount = mRepeatMax) {
                setActive(false);
                return;
            }
            
            if (mRowWeepleCount == mParameters.getInt("weeple_count_" + mRow), 1) {
                mRow++;
                mRowWeepleCount = 0;
            }
            
            if (!mParameters.contains("weeple_type_" + mRow)) {
                mRepeatCount++;
                mRow = 0;
                mRowWeepleCount = 0;
                continue;
            }
        
            try {
                weepleType = WeepleType.valueOf(mParameters.getString("weeple_type_" + mRow));
            } catch (IllegalArgumentException e) {  
            }
            break;
        }
        
        if (weepleType != null) {
            ElementParameter weepleParameters = new ElementParameter();
            weepleParameters.x = mX;
            weepleParameters.y = mY;
            weepleParameters.set(ElementParameter.FLOOR, mFloor);
            weepleParameters.set(ElementParameter.WEEPLE_TYPE, weepleType);
            
            getLevel().addWeeple(new Weeple(getLevel(), weepleParameters));
        }
        
        mWeepleCount++;
     }
}

Het gaat me voornamelijk om de while-lus, stiekem is dit gewoon een verkapte vorm van GoTo gebruik. Is dit af te raden en zo ja, welke richtnig moet ik op denken?

[xh3adshotx]

Edit: nevermind, het is vrijdagmiddag... Kijk naar de code van @IceM

[ Voor 97% gewijzigd door xh3adshotx op 09-05-2014 16:08 ]

[Herko_ter_Horst]

De eerste vraag die bij me opkomt: waarom is het een zo'n rare map i.p.v. een lijst van WeepleDefinition o.i.d. (m.a.w. een fatsoenlijk object dat weergeeft wat je er mee wilt)?

[MichielPH]

Herko_ter_Horst schreef op vrijdag 09 mei 2014 @ 15:56:
De eerste vraag die bij me opkomt: waarom is het een zo'n rare map i.p.v. een lijst van WeepleDefinition (m.a.w. een fatsoenlijk object dat weergeeft wat je er mee wilt)?

Omdat ik levels maak met een externe editor, wat uiteindelijk als.json-bestand wordt opgeslagen. Ik moet het in eerste instantie dus definiëren op basis van Strings en integers.

[IceM]

break; continue; return; etc statements maken het soms wat lastig om de flow te volgen. Als het eenvoudig te vermijden is zou ik dat doen. Je code komt volgens mij neer op dit (even snel gekeken):

Java:

    public void createWeeple() { 
        WeepleType weepleType = null; 
        while (null == weepleType && mRepeatCount != mRepeatMax) {              
            if (mRowWeepleCount == mParameters.getInt("weeple_count_" + mRow), 1) { 
                mRow++; 
                mRowWeepleCount = 0; 
            } 
             
            if (!mParameters.contains("weeple_type_" + mRow)) { 
                mRepeatCount++; 
                mRow = 0; 
                mRowWeepleCount = 0; 
            }  else {     
                try { 
                    weepleType = WeepleType.valueOf(mParameters.getString("weeple_type_" + mRow)); 
                } catch (IllegalArgumentException e) {   
                    // Exceptions opeten is gevaarlijk/code smell!
                } 
            }
        }

        if (mRepeatCount == mRepeatMax) {  // er stond = maar moet denk ik == zijn?
            setActive(false); 
        } else {
            ElementParameter weepleParameters = new ElementParameter(); 
            weepleParameters.x = mX; 
            weepleParameters.y = mY; 
            weepleParameters.set(ElementParameter.FLOOR, mFloor); 
            weepleParameters.set(ElementParameter.WEEPLE_TYPE, weepleType); 
             
            getLevel().addWeeple(new Weeple(getLevel(), weepleParameters)); 
             
            mWeepleCount++; 
        }
     } 

[NMe]

MichielPH schreef op vrijdag 09 mei 2014 @ 16:00:
[...]
Omdat ik levels maak met een externe editor, wat uiteindelijk als.json-bestand wordt opgeslagen. Ik moet het in eerste instantie dus definiëren op basis van Strings en integers.

In de eerste instantie, maar dat kun je toch mooie weg-abstraheren?

Ontopic: ik vind het een ranzige constructie, maar je alternatief is niet veel beter als je wil blijven repeaten. Je zou dan met een variabele moeten werken die bijhoudt of je al dan niet uit de loop moet springen.

Overigens wil je even naar regel 27 kijken, volgens mij bedoel je daar == en niet =. Bovendien zou ik liever >= gebruiken, stukje defensief programmeren.

[MichielPH]

xh3adshotx en IceM, beiden bedankt en ik ga ze bekijken!

NMe schreef op vrijdag 09 mei 2014 @ 16:06:
[...]

In de eerste instantie, maar dat kun je toch mooie weg-abstraheren?

Dat abstraheren gebeurt toch juist hier? Hoeveel toegevoegde waarde heeft het dan om het resultaat eerst in het geheugen te zetten en alsnog uit te lezen?

Ontopic: ik vind het een ranzige constructie, maar je alternatief is niet veel beter als je wil blijven repeaten. Je zou dan met een variabele moeten werken die bijhoudt of je al dan niet uit de loop moet springen.

Lijkt dat ik IceM's consturctie overneem.

Overigens wil je even naar regel 27 kijken, volgens mij bedoel je daar == en niet =. Bovendien zou ik liever >= gebruiken, stukje defensief programmeren.

Jep, beiden meegenomen.

[NMe]

MichielPH schreef op vrijdag 09 mei 2014 @ 16:13:
Dat abstraheren gebeurt toch juist hier? Hoeveel toegevoegde waarde heeft het dan om het resultaat eerst in het geheugen te zetten en alsnog uit te lezen?

Nouja, ik doelde meer op een class Weeple, eventueel met een static createFromJSON-method. Je hebt het hele JSON-verhaal dan weggestopt en dus hoef je in je daadwerkelijke programmacode alleen te weten hoe je Weeple-class werkt.

[Caelorum]

Sowieso makkelijk mocht het ooit zo zijn dat je een ander formaat krijgt voorgeschoteld omdat je een andere editor gebruikt of je eigen hebt gemaakt.

[Daos]

1 Alle params in 1 HashMap opslaan met eigen gemaakte String-keys om alles maar kwijt te kunnen vind ik lelijk en is waarschijnlijk lastig te debuggen. Gebruik netjes een klasse om het type aan het aantal te koppelen. (Of als je die null niet nodig hebt en types 1 keer voor kunnen komen een HashMap<String, Integer>)

2 Volgens mij kan die loop simpeler (edit: en zonder while als je vooraf ongeldige counts eruit filtered). Zie code hieronder.

3 Als die Weeples bij elk type andere dingen opslaan of doen (al is het maar een eigen fysiek kleurtje) kan je beter inheritance gaan gebruiken.

code (nog geen setters/getters en checks; repeat heeft 'normale' betekenis gekregen)

Java:

import java.util.*;

class DeepIterator<T> implements Iterator<T> {
    private Iterator<? extends Iterable<T>> outerIterator;
    private Iterator<T> innerIterator;

    public DeepIterator(Iterator<? extends Iterable<T>> iterator) {
        outerIterator = iterator;
    }

    private boolean isIteratorValid() {
        return innerIterator != null && innerIterator.hasNext();
    }

    private void prepareValidNext() {
        while (outerIterator.hasNext() && !isIteratorValid()) {
            innerIterator = outerIterator.next().iterator();
        }
    }
    
    @Override
    public boolean hasNext() {
        prepareValidNext();
        return isIteratorValid();
    }

    @Override
    public T next() {
        prepareValidNext();

        if (!hasNext()) {
            throw new NoSuchElementException();
        }
        
        return innerIterator.next();
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }
}

class Quantity<T> implements Iterable<T> {
    T object;
    int count;
    
    Quantity(T object, int count) {
        this.object = object;
        this.count = count;
    }

    @Override
    public Iterator<T> iterator() {
        return new QuantityIterator<T>(this);
    }
}

class QuantityIterator<T> implements Iterator<T> {
    private Quantity<T> quantity;
    private int item = 0;

    public QuantityIterator(Quantity<T> quantity) {
        this.quantity = quantity;
    }

    @Override
    public boolean hasNext() {
        return item < quantity.count;
    }

    @Override
    public T next() {
        if (!hasNext()) {
            throw new NoSuchElementException();
        }
        
        item++;
        return quantity.object;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }
}

class QuantityList<T> implements Iterable<T> {
    List<Quantity<T>> list;
    
    public QuantityList(List<Quantity<T>> list) {
        this.list = list;
    }
    
    @Override
    public Iterator<T> iterator() {
        return new DeepIterator<T>(this.list.iterator());
    }
}

class WeepleStorage implements Iterable<String> {
    Quantity<QuantityList<String>> weeples;
    
    @Override
    public Iterator<String> iterator() {
        return new DeepIterator<String>(this.weeples.iterator());
    }
}

class Entrance { 
    Iterator<String> wsi;
    
    Entrance(WeepleStorage storage) { 
        wsi = storage.iterator();
    } 
    
    void onTick() { 
        createWeeple(); 
    } 

    void createWeeple() {
        if (!wsi.hasNext()) {
            return;
        }
        
        String weeple = wsi.next();
        System.out.print(weeple + " ");
    }
}

public class WeepleMain {
    public static void main(String[] args) {
        // fill params
        Vector<Quantity<String>> weeples = new Vector<Quantity<String>>();
        weeples.add(new Quantity<String>("Orange", 2));
        weeples.add(new Quantity<String>(null, 1));
        weeples.add(new Quantity<String>("Black", 2));
        
        WeepleStorage storage = new WeepleStorage();
        int repeat = 2;
        storage.weeples = new Quantity<QuantityList<String>>(new QuantityList<String> (weeples), repeat);

        // print q using foreach
        for (String weeple : new Quantity<String>("Blue", 5)) {
            System.out.print(weeple + " ");
        }
        System.out.println();

        // print ql using foreach
        for (String weeple : new QuantityList<String>(weeples)) {
            System.out.print(weeple + " ");
        }
        System.out.println();
        
        // print storage using foreach
        for (String weeple : storage) {
            System.out.print(weeple + " ");
        }
        System.out.println();
        
        // simulate
        Entrance entrance = new Entrance(storage);
        int ticks = 15;
        for (int i = 0; i < ticks; i++) {
            entrance.onTick();
        }
        System.out.println();
    }
}

edit:
Mijn oplossing lijkt 3x zo groot, maar lijkt mij veel beter. Ik heb de zooi om de volgende weeple te krijgen uit Entrance gehaald. Door de iterators kan je ook later makkelijk besluiten of je ze stuk voor stuk op wilt halen of in een foreach en kan je ze ook makkelijk in andere klassen gebruiken.Ook werkt het bij niet-positieve counts en lege params. Verder zijn de methoden simpeler en zijn de klassen en methoden makkelijker (apart) te testen.

edit zoveel:
Ik heb nu 1 simpele while en 1 simpele if (+ 2 exceptiongooiers die ik niet meetel). Allen met een body van 1 statement. Verder geen continue, break of andere rare fratsen. Simpeler dan dit kan bijna niet.

[ Voor 99% gewijzigd door Daos op 11-05-2014 14:26 . Reden: Probleem niet goed begrepen + ~10 edits/fixes daarna (hoop dat er geen emails verstuurd worden) ]

[MichielPH]

Daos schreef op zaterdag 10 mei 2014 @ 22:42:
1 Alle params in 1 HashMap opslaan met eigen gemaakte String-keys om alles maar kwijt te kunnen vind ik lelijk en is waarschijnlijk lastig te debuggen. Gebruik netjes een klasse om het type aan het aantal te koppelen. (Of als je die null niet nodig hebt en types 1 keer voor kunnen komen een HashMap<String, Integer>)
2 Volgens mij kan die loop simpeler (edit: en zonder while als je vooraf ongeldige counts eruit filtered). Zie code hieronder.

3 Als die Weeples bij elk type andere dingen opslaan of doen (al is het maar een eigen fysiek kleurtje) kan je beter inheritance gaan gebruiken.

code (nog geen setters/getters en checks; repeat heeft 'normale' betekenis gekregen)

Java:

import java.util.*;

class DeepIterator<T> implements Iterator<T> {
    private Iterator<? extends Iterable<T>> outerIterator;
    private Iterator<T> innerIterator;

    public DeepIterator(Iterator<? extends Iterable<T>> iterator) {
        outerIterator = iterator;
    }

    private boolean isIteratorValid() {
        return innerIterator != null && innerIterator.hasNext();
    }

    private void prepareValidNext() {
        while (outerIterator.hasNext() && !isIteratorValid()) {
            innerIterator = outerIterator.next().iterator();
        }
    }
    
    @Override
    public boolean hasNext() {
        prepareValidNext();
        return isIteratorValid();
    }

    @Override
    public T next() {
        prepareValidNext();

        if (!hasNext()) {
            throw new NoSuchElementException();
        }
        
        return innerIterator.next();
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }
}

class Quantity<T> implements Iterable<T> {
    T object;
    int count;
    
    Quantity(T object, int count) {
        this.object = object;
        this.count = count;
    }

    @Override
    public Iterator<T> iterator() {
        return new QuantityIterator<T>(this);
    }
}

class QuantityIterator<T> implements Iterator<T> {
    private Quantity<T> quantity;
    private int item = 0;

    public QuantityIterator(Quantity<T> quantity) {
        this.quantity = quantity;
    }

    @Override
    public boolean hasNext() {
        return item < quantity.count;
    }

    @Override
    public T next() {
        if (!hasNext()) {
            throw new NoSuchElementException();
        }
        
        item++;
        return quantity.object;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }
}

class QuantityList<T> implements Iterable<T> {
    List<Quantity<T>> list;
    
    public QuantityList(List<Quantity<T>> list) {
        this.list = list;
    }
    
    @Override
    public Iterator<T> iterator() {
        return new DeepIterator<T>(this.list.iterator());
    }
}

class WeepleStorage implements Iterable<String> {
    Quantity<QuantityList<String>> weeples;
    
    @Override
    public Iterator<String> iterator() {
        return new DeepIterator<String>(this.weeples.iterator());
    }
}

class Entrance { 
    Iterator<String> wsi;
    
    Entrance(WeepleStorage storage) { 
        wsi = storage.iterator();
    } 
    
    void onTick() { 
        createWeeple(); 
    } 

    void createWeeple() {
        if (!wsi.hasNext()) {
            return;
        }
        
        String weeple = wsi.next();
        System.out.print(weeple + " ");
    }
}

public class WeepleMain {
    public static void main(String[] args) {
        // fill params
        Vector<Quantity<String>> weeples = new Vector<Quantity<String>>();
        weeples.add(new Quantity<String>("Orange", 2));
        weeples.add(new Quantity<String>(null, 1));
        weeples.add(new Quantity<String>("Black", 2));
        
        WeepleStorage storage = new WeepleStorage();
        int repeat = 2;
        storage.weeples = new Quantity<QuantityList<String>>(new QuantityList<String> (weeples), repeat);

        // print q using foreach
        for (String weeple : new Quantity<String>("Blue", 5)) {
            System.out.print(weeple + " ");
        }
        System.out.println();

        // print ql using foreach
        for (String weeple : new QuantityList<String>(weeples)) {
            System.out.print(weeple + " ");
        }
        System.out.println();
        
        // print storage using foreach
        for (String weeple : storage) {
            System.out.print(weeple + " ");
        }
        System.out.println();
        
        // simulate
        Entrance entrance = new Entrance(storage);
        int ticks = 15;
        for (int i = 0; i < ticks; i++) {
            entrance.onTick();
        }
        System.out.println();
    }
}

edit:
Mijn oplossing lijkt 3x zo groot, maar lijkt mij veel beter. Ik heb de zooi om de volgende weeple te krijgen uit Entrance gehaald. Door de iterators kan je ook later makkelijk besluiten of je ze stuk voor stuk op wilt halen of in een foreach en kan je ze ook makkelijk in andere klassen gebruiken.Ook werkt het bij niet-positieve counts en lege params. Verder zijn de methoden simpeler en zijn de klassen en methoden makkelijker (apart) te testen.

edit zoveel:
Ik heb nu 1 simpele while en 1 simpele if (+ 2 exceptiongooiers die ik niet meetel). Allen met een body van 1 statement. Verder geen continue, break of andere rare fratsen. Simpeler dan dit kan bijna niet.

Mooie, heldere oplossing. Echter, nu ga je wel voorbij aan het feit dat stap 1 van het proces sowieso een uit te lezen bestand is. De implementatie hiervaan geeft me de mogelijkheid om eigenschappen aan een element zoals Entrance mee te geven doormiddel van String-waarde-paren, waarvan de volgorde niet gegarandeerd is.

Code wat volgt is dus niet drect mogelijk, omdat ik hoe dan ook altijd begin met String-waarde-paren.

Java:

weeples.add(new Quantity<String>("Orange", 2));
weeples.add(new Quantity<String>(null, 1));
weeples.add(new Quantity<String>("Black", 2));

Lelijke lussen zal ik dan uit de weg gaan door de String-waarde-paren eerst te vertalen naar een sequentie van weeples en daar door te itereren.

[Daos]

MichielPH schreef op maandag 12 mei 2014 @ 14:02:
[...]
Mooie, heldere oplossing. Echter, nu ga je wel voorbij aan het feit dat stap 1 van het proces sowieso een uit te lezen bestand is. De implementatie hiervaan geeft me de mogelijkheid om eigenschappen aan een element zoals Entrance mee te geven doormiddel van String-waarde-paren, waarvan de volgorde niet gegarandeerd is.

Code wat volgt is dus niet drect mogelijk, omdat ik hoe dan ook altijd begin met String-waarde-paren.

Java:

weeples.add(new Quantity<String>("Orange", 2));
weeples.add(new Quantity<String>(null, 1));
weeples.add(new Quantity<String>("Black", 2));

Lelijke lussen zal ik dan uit de weg gaan door de String-waarde-paren eerst te vertalen naar een sequentie van weeples en daar door te itereren.

Dat je data uit een bestand in een bepaald formaat komt zou er niet toe mogen doen. Straks blijkt key-value niet geschikt (traag, onhandig) en wil je een xml en nog later een database gebruiken.

Je kan het opslaan/uitlezen beter in een persistence-laag/layer stoppen zodat je in de toekomst alleen dat deel hoeft aan te passen en niet je hele programma. Ook programmeert het lekkerder/vrijer als je niet steeds met het bestandsformaat rekening hoeft te houden.

Je kan nu het beste direct op het moment dat je die key-value pairs inleest uit je bestand die omzetten naar een datastructuur die handig werkt in je programma zoals bv de Quantity en daar verder mee werken. Entrance en ook Quantity zouden geen weet moeten hebben van die key-value pairs die je nu gebruikt.

[creator1988]

Ik vind het echt niet zo ranzig hoor. De code is netjes procedureel, en makkelijk te volgen. Als je er dan weer 40 abstractielagen overheen gaat gooien voor 'het geval dat' (wat nooit gaat komen), wordt het een grote zooi.

Anyway, kan je over de keys van een hashmap itereren? Dan zoiets.

code:

var weeples = [];
var keys = Object.keys(map);
while(keys.length > 0) {
  var type = keys.splice(0, 1);
  if (type == 'repeat') { /* do something */ }
  var count = keys.splice(0, 1);
  for (var i = 0; i < count; i++)
    weeples.push(type);
}

[ Voor 43% gewijzigd door creator1988 op 14-05-2014 10:36 ]