[JAVA] iterator exception

[clooner]

Dit is mijn code en hiermee krijg ik een IlligalStateException omdat ik "remove" twee keer aanroep zonder een "next" ertussen. Toch zou dit volgens mij moeten werken.

De positie van het object in listB is altijd verder dan die van listA. Als ik het huidige object in listB dus eerst verwijder gebeurd er niks met de positie van het huidige object in listA. Jammer genoeg als ik dus het huidige object in listA probeer te verwijderen krijg ik de exception terwijl er helemaal niks is veranderd met het object in listA. Weet iemand hier een oplossing voor of een andere goede manier om in de lijst twee objecten te vergelijken er dan iets mee te doen om tenslotte verwijderd te worden.

Java:

public void run()
{
    System.out.println("Executing the matching!");
    Iterator<Contestant> listA = contestantList.iterator();
    while (listA.hasNext() && contestantList.size()>1)
    {
        boolean matched=false;
        Contestant contestantA = listA.next();
        Iterator<Contestant> listB = listA;
        while ((listB.hasNext()) && (contestantList.size()>1) && (!matched))
        {
            Contestant contestantB = listB.next();
            if (isMatch(contestantA,contestantB))
            {
                matched=true;
                // do something with them here...
                listB.remove();
                listA.remove();
            }
        }
    }
}

[Elvis]

Ben je niet met je beide iterators(listA & listB) dezelfde list aan het doorlopen?
Als je dan een object uit die list verwijderd dmv de de listB iterator, kan je hem toch niet nóg eens verwijderen dmv de listA iterator...

van de Java API:

Iterator<E>
remove

void remove()

    Removes from the underlying collection the last element returned by the iterator (optional operation). This method can be called only once per call to next. The behavior of an iterator is unspecified if the underlying collection is modified while the iteration is in progress in any way other than by calling this method.

    Throws:
        UnsupportedOperationException - if the remove operation is not supported by this Iterator. 
        IllegalStateException - if the next method has not yet been called, or the remove method has already been called after the last call to the next method.

[Janoz]

Je maakt hier een enorme denkfout. Het simpel toekennen van een iterator aan een andere variabele maakt er nog geen kopie van. listA en listB zijn beiden hetzelfde. Het is niet ineens alsof je een neiwue iterator hebt die eventjes in een aparte lus vanaf dat moment verder kijkt. Een aanroep als listB.next() zorgt er ook gewoon voor dat de iterator 'listA' een stapje verder gaat.

De foutmelding is dus compleet logisch. Aangezien listA en listB hetzelfde object zijn worden de remove aanroepen ook op dezelfde iterator uitgevoerd en heb je dus precies de situatie waarvan jij denkt dat dat niet het geval is .

Sowieso vind ik het een niet zo handig gekozen naamgeving. Je noemt ze wel list, maar het zijn geen lijsten, maar iterators.

[clooner]

Janoz schreef op woensdag 19 november 2008 @ 11:55:
Je maakt hier een enorme denkfout. Het simpel toekennen van een iterator aan een andere variabele maakt er nog geen kopie van. listA en listB zijn beiden hetzelfde. Het is niet ineens alsof je een neiwue iterator hebt die eventjes in een aparte lus vanaf dat moment verder kijkt. Een aanroep als listB.next() zorgt er ook gewoon voor dat de iterator 'listA' een stapje verder gaat.

Een denkfout is het inderdaad... Had niet door dat listA en listB gelijk zijn.

maar als ik dit doe krijg ik een ConcurrentModificationException

Java:

Iterator<Contestant> listA = contestantList.iterator();
Iterator<Contestant> listB = contestantList.iterator();

Ik zie wel een oplossing door in plaats van ze te verwijderen te markeren om verwijderd te worden. Zodra ik dan iets vindt te stoppen en opnieuw loopen totdat ik de gemarkeerde vind en deze dan te verwijderen. En daarna weer overnieuw te beginnen maar dan moet ik oneindig vaak door die lijst heenlopen. En dat lijkt me niet de beste oplossing. Hoe kan zoeken op 2 verschillende plaatsen en zodra iets gevonden dat dan ook direct verwijderen?

[Janoz]

Die ConcurrentModificationException komt omdat de achterliggende lijst aangepast wordt door iets anders dan de iterator zelf (immers, de andere iterator).

Je originele idee kan best werken, maar dan moet je niet met iterators gaan werken, maar gewoon indexen. Je moet wel goed opletten dat je ze goed begrenst (de lengte van de lijst veranderd tijdens het doorlopen), en dat je ze op het juiste moment ophoogt (wanneer je een element verwijderd hoef je de index niet op te hogen om bij het volgende element uit te komen).

[Jaap-Jan]

Janoz schreef op woensdag 19 november 2008 @ 12:23:
Die ConcurrentModificationException komt omdat de achterliggende lijst aangepast wordt door iets anders dan de iterator zelf (immers, de andere iterator).

Je originele idee kan best werken, maar dan moet je niet met iterators gaan werken, maar gewoon indexen. Je moet wel goed opletten dat je ze goed begrenst (de lengte van de lijst veranderd tijdens het doorlopen), en dat je ze op het juiste moment ophoogt (wanneer je een element verwijderd hoef je de index niet op te hogen om bij het volgende element uit te komen).

Om dat gezeur met indexen te voorkomen, kun je dan het beste achteraan beginnen, dan veranderen de indexen niet ineens tijdens het verwijderen en is de check niet meer dan controleren op index >= 0.

[ Voor 17% gewijzigd door Jaap-Jan op 19-11-2008 12:33 ]

[Salandur]

volgens mij ben je aan het ontdubbelen. Dat kan je beter op een andere manier doen dan dat je nu doet. het is bijvoorbeeld niet toegestaan om een iterator.next() aan te roepen als je bijvoorbeeld list.delete(..) hebt aangeroepen.

[clooner]

Ik heb dit even snel in elkaar gezet en mijn unit test werkt er goed op Ik zat gewoon teveel gefocused op het gebruik van die iterator...

Java:

public void run()
{
    System.out.println("Executing the matching!");
    for(int indexA=0; indexA < contestantList.size();)
    {
        boolean matched=false;
        Contestant contestantA = contestantList.get(indexA);
        for(int indexB=indexA; indexB < contestantList.size();)
        {
            Contestant contestantB = contestantList.get(indexB);
            if (isMatch(contestantA,contestantB))
            {
                // Yuppie
                matched=true;
                contestantList.remove(indexB);
                contestantList.remove(indexA);
            }
            if (!matched)
                indexB++;
            else
                break;
        }
        if (!matched)
            indexA++;
    }
}

[Jaap-Jan]

Ik bedoel trouwens op deze manier:

Java:

public void run()
{
    System.out.println("Executing the matching!");
    for(int indexA=contestantList.size(); indexA >= 0; indexA--)
    {
        Contestant contestantA = contestantList.get(indexA);
        for(int indexB=indexA; indexB >= 0; indexB--)
        {
            Contestant contestantB = contestantList.get(indexB);
            if (isMatch(contestantA,contestantB))
            {
                // Yuppie
                contestantList.remove(indexB);
                contestantList.remove(indexA);
            }
        }
    }
}

Dan heb je die extra checks en die 'matched' boolean niet meer nodig.

Salandur schreef op woensdag 19 november 2008 @ 12:39:
volgens mij ben je aan het ontdubbelen. Dat kan je beter op een andere manier doen dan dat je nu doet. het is bijvoorbeeld niet toegestaan om een iterator.next() aan te roepen als je bijvoorbeeld list.delete(..) hebt aangeroepen.

Ik vind het niet op ontdubbelen lijken, want dan verwijder je ze niet allebei. En als het om ontdubbelen ging, zou ik er al voor zorgen dat er geen duplicaten kunnen zijn, door een Set te gebruiken.

[ Voor 38% gewijzigd door Jaap-Jan op 19-11-2008 12:59 ]

[Herko_ter_Horst]

Heeft de "Contestant" class geen equals() die hetzelfde doet als "isMatch"? Dan kun je simpel ontdubbelen door de list in een Set (LinkedHashSet als je de volgorde wilt bewaren) te drukken en die set vervolgens weer in een List.

Moet het trouwens uberhaupt wel een List zijn?

[Elvis]

Herko_ter_Horst schreef op woensdag 19 november 2008 @ 12:59:
Heeft de "Contestant" class geen equals() die hetzelfde doet als "isMatch"? Dan kun je simpel ontdubbelen door de list in een Set (LinkedHashSet als je de volgorde wilt bewaren) te drukken en die set vervolgens weer in een List.

Moet het trouwens uberhaupt wel een List zijn?

Ik neem aan dat de de isMatch() methode van Contestant eigenlijk een equals() methode is.

In plaats van een methode te maken om dubbele entries te verwijderen, is het imo wél makkelijker om een methode te voorzien die geen dubbele entries toestaat in de list. Dan heb je dit helemaal niet nodig...

@hieronder: toegegeven een set zou idd beter werken. Maar als je nou per se een list wilt gebruiken...

[ Voor 7% gewijzigd door Elvis op 19-11-2008 15:00 ]

[Jaap-Jan]

Elvis schreef op woensdag 19 november 2008 @ 14:27:
[...]


Ik neem aan dat de de isMatch() methode van Contestant eigenlijk een equals() methode is.

In plaats van een methode te maken om dubbele entries te verwijderen, is het imo wél makkelijker om een methode te voorzien die geen dubbele entries toestaat in de list. Dan heb je dit helemaal niet nodig...

Een Set, dus, hoef je helemaal niets zelf voor te coden. Maar nogmaals, sinds wanneer haal je bij het verwijderen van duplicaten beide weg?

[bomberboy]

Bij de nieuwe code van clooner blijft in het geval dat een object 3 maal in de lijst zit er nog altijd eentje plakken. Er is immers een break die uit de binnenste for springt na de eerste match.

De oplossing van Jaap-Jan heeft dit probleem dan weer niet, maar zal in dat geval wel een keer te veel .remove(contestantA) uitvoeren. Op zich geen drama, maar ook niet ideaal.

In ieder geval lijkt een andere datastructuur hier meer aangewezen. Al valt daar natuurlijk niet veel over te zeggen zonder te weten waarvoor dit alles moet dienen natuurlijk.

[Herko_ter_Horst]

Elvis schreef op woensdag 19 november 2008 @ 14:27:
[...]
@hieronder: toegegeven een set zou idd beter werken. Maar als je nou per se een list wilt gebruiken...

De vraag is dan waarom je een list wilt.

Ik zie vaak dat lists gebruikt worden omdat je in de UI nou eenmaal graag iets in een voorspelbare volgorde wilt tonen aan de gebruiker. In de businesslaag worden er dan allerlei noodgrepen toegepast om op dit list het gedrag dat bij het datamodel hoort te simuleren, zoals uniciteit van entities zoals in dit geval; of mapping: twee "synchrone" lists, waar eigenlijk gewoon een Map gebruikt had moeten worden, etc.

De vraag is dus: is het gebruik van die list een presentatie-issue of een datamodel-issue? Kun je de volgordelijkheid van de list ook opvangen met een SortedSet of een LinkedHashSet? Wil je persé random access op index of is een Map misschien beter? Etc, etc, etc.

[clooner]

Java:

class Contestant {
    public int connectionId;
    public String name;
    public int points;
    public int timeout;
    public int minAmount;
    public int maxAmount;
    public int ticker;
    public Contestant() {}
}

Java:

Vector<Contestant> contestantList = new Vector<Contestant>();

Die methode van jaap-jan ga ik morgen direct proberen... Ziet er iets netter uit

contestantA en contestantB ga ik dan met een eigen methode isMatch vergelijken. Zijn namelijk niet exact hetzelfde. Deze methode geeft een boolean waarde terug als ze dicht genoeg in de buurt komen.

Is er een betere manier om de contestantList op te bouwen?

[BalusC]

Laat die klasse de Comparable interface implementeren en voer hetgeen je in jouw zelfverzonnen isMatch() methode gebruikt in de compareTo() methode en gebruik tenslotte een TreeSet om de objecten te verzamelen en automagisch te laten sorteren bij elke toevoegine.

[Herko_ter_Horst]

BalusC, volgens mij is dat niet waar clooner naar op zoek is.

Als ik het goed begrijp is het criterium niet dat ContestantA en ContestantB "equal" zijn, maar moet er een soort "fuzzy" matching aan te pas komen om te bepalen of ze dicht genoeg bij elkaar zitten om uit de lijst geknikkerd te worden.

clooner, kun je nog iets meer zeggen over de bedoeling van het geheel, los van een mogelijke oplossing?

[clooner]

BalusC schreef op woensdag 19 november 2008 @ 16:53:
Laat die klasse de Comparable interface implementeren en voer hetgeen je in jouw zelfverzonnen isMatch() methode gebruikt in de compareTo() methode en gebruik tenslotte een TreeSet om de objecten te verzamelen en automagisch te laten sorteren bij elke toevoegine.

ik ben niet bekend met compareTo() maar name,points,timeout moeten exact hetzelfde zijn. Dan heb je de A.minAmount<B.maxAmount en A.maxAmount>B.minAmount. Ticker is niet van belang.
Kan je dat ook met compareTo() doen?

[Jaap-Jan]

bomberboy schreef op woensdag 19 november 2008 @ 15:47:
(..)
De oplossing van Jaap-Jan heeft dit probleem dan weer niet, maar zal in dat geval wel een keer te veel .remove(contestantA) uitvoeren. Op zich geen drama, maar ook niet ideaal.
(..)

Dat is wel een ramp trouwens, want hij verwijderd een element op een bepaalde index en als er eerder al één verwijderd is, wordt het vorige onderzochte element verwijderd. Niet geheel de bedoeling en stomweg fout. Wel fixen trouwens.

Maar dat een reeds verwijderd element proberen te verwijderen 'niet ideaal' is vind ik een beetje overdreven. Het is goed gedocumenteerd gedrag. Sterker nog: er wordt een boolean geretourneerd of het element aanwezig was of niet, dus dat wekt de indruk dat de API er rekening mee houdt dat dat wel eens voor kan komen.

Ik zit m'n code eens te testen, maar volgens mij klopte er niet veel van, door wat stomme fouten.

clooner schreef op woensdag 19 november 2008 @ 17:10:
[...]


ik ben niet bekend met compareTo() maar name,points,timeout moeten exact hetzelfde zijn. Dan heb je de A.minAmount<B.maxAmount en A.maxAmount>B.minAmount. Ticker is niet van belang.
Kan je dat ook met compareTo() doen?

Het kan wel, maar het wordt niet aangeraden in Comparable.compareTo(). Ten minste, equals() en compareTo() zullen in jouw implementatie verschillende uitkomsten geven en dat is sterk afgeraden.

Dat maakt het trouwens niet triviaal om Set te gebruiken, want 'duplicaten' in de letterlijke zin zijn het niet. En om equals() te misbruiken om een Set te kunnen gebruiken is ook niet handig.

[ Voor 93% gewijzigd door Jaap-Jan op 19-11-2008 19:49 ]

[Herko_ter_Horst]

Met ^^

Het gebruik van een Set heeft alleen zin als je de equals() op een fatsoenlijke manier kunt implementeren.

Ik heb al eens ergens een topic gezien over iets als een Equalator, die analoog aan een Comparator voor verschillende situaties verschillende "equality" kan representeren. Dat zou in dit geval handig zijn (als er collections waren die daarmee om konden gaan).