Treeset doesn't remove object - Java

Question

I'm using Treeset in my project to store an Object class (that i've created early).

I've also implemented build() method that have to add in my treeset the Object and it works great. Now I' ve to implement demolish() method (it has to remove the object specified as parameter), but i've problem: it doesn't remove it:

 private void demolish(int y, String p){
    Iterator iterator = alberobello.iterator();
    while(iterator.hasNext()){
        Edificio edificio = (Edificio) iterator.next();
        if(edificio.getPinodeipalazzi().equals(p) && edificio.getAnno() == y){
            alberobello.remove(edificio);
            dimension --;
            System.out.println("Removed: " + edificio.getPinodeipalazzi()+ " " + edificio.getAnno() + " " +alberobello.size() );
        }
    }
}

This is ALBEROBELLO Declaration

private static TreeSet<Edificio> alberobello;

private static int dimension;

private Skyline(){

    alberobello = new TreeSet<Edificio>();
    dimension = 0;

}

this is EDIFICIO Class

public class Edificio implements Comparable, Iterable{

private int anno;
private String pinodeipalazzi;
private String lato;
private int distanza;
private int base;
private int altezza;

public Edificio(int y, String p, String l, int d, int b, int h){
    this.anno = y;
    this.pinodeipalazzi = p;
    this.lato = l;
    this.distanza = d;
    this.base = b;
    this.altezza = h;
}

public int getAltezza() {
    return altezza;
}

public int getAnno() {
    return anno;
}

public int getBase() {
    return base;
}

public int getDistanza() {
    return distanza;
}

public String getLato() {
    return lato;
}

public String getPinodeipalazzi() {
    return pinodeipalazzi;
}

@Override
public int compareTo(Object o) {
    if((o == null) || this.distanza > ((Edificio)o).distanza)
        return 1;
    /*else if(this.distanza == ((Edificio)o).distanza)
        return 0;*/
    else
        return -1;

}


@Override
public Iterator iterator() {
    return new Iterator() {
        @Override
        public boolean hasNext() {
            return false;
        }

        @Override
        public Object next() {
            return null;
        }
    };
}

@Override
public void forEach(Consumer action) {

}

Please show the declaration of `alberobello`. – Eran May 17 '18 at 08:38 — Eran, May 17 '18 at 08:38
..and the `Edificio` class – Jack Flamp May 17 '18 at 08:41 — Jack Flamp, May 17 '18 at 08:41

Eran · Answer 1 · 2018-05-17T09:03:01.697

3

Your compareTo method is the source of the problem:

public int compareTo(Object o) {
    if((o == null) || this.distanza > ((Edificio)o).distanza)
        return 1;
    /*else if(this.distanza == ((Edificio)o).distanza)
        return 0;*/
    else
        return -1;

}

Since it never returns 0, the element you are trying to remove from the TreeSet is not found.

I'm not sure why you commented out the part that returns 0, but you should uncomment it:

public int compareTo(Object o) {
    if((o == null) || this.distanza > ((Edificio)o).distanza)
        return 1;
    else if(this.distanza == ((Edificio)o).distanza)
        return 0;
    else
        return -1;
}

P.S. It would be better not to use raw Comparable. Change your class to:

public class Edificio implements Comparable<Edificio>
{
    ...
    @Override
    public int compareTo(Edificio o) {
        if((o == null) || this.distanza > o.distanza)
            return 1;
        else if(this.distanza == o.distanza)
            return 0;
        else
            return -1;

    }
    ...
}

EDIT: Another issue with your code, which was hidden by the failure of remove() to remove the element, is that you are calling alberobello.remove(edificio) while iterating over the Set. That is likely to throw ConcurrentModificationException once you fix your compareTo method. You should simply use the Iterator's remove() method:

    if(edificio.getPinodeipalazzi().equals(p) && edificio.getAnno() == y){
        iterator.remove();
        dimension --;
        System.out.println("Removed: " + edificio.getPinodeipalazzi()+ " " + edificio.getAnno() + " " +alberobello.size() );
    }

edited May 17 '18 at 09:03

answered May 17 '18 at 08:53

Eran

387,369
54
702
768

doesn't he have to implement equals and hashcode to make sure that the object is found in set? – Jack Flamp May 17 '18 at 08:58
@JackFlamp Probably not when you use the TreeSet and implement the comparable interface. The point of that set is **sorting**. You need the <= relation for that. It is not just a hashset/map that does == or != – GhostCat May 17 '18 at 08:59
@JackFlamp No, in TreeSet `hashCode` and `equals` are not used. Only `compareTo` (or `compare` in case a `Comparator` is used) – Eran May 17 '18 at 09:00
yes, i've commented it because of ConcurrentModificationException, i've solved uncomment it and using iterator.remove – Sab Biuzzino May 17 '18 at 09:03
@Eran are you sure? https://docs.oracle.com/javase/8/docs/api/java/util/TreeSet.html#remove-java.lang.Object-. "removes an element e such that (o==null ? e==null : o.equals(e))" – Jack Flamp May 17 '18 at 09:07
@JackFlamp well, the doc also says `Note that the ordering maintained by a sorted set (whether or not an explicit comparator is provided) must be consistent with equals **if the sorted set is to correctly implement the Set interface**.` This means that `remove()` will still work (without overriding `equals`) if the compareTo/compare method is implemented correctly, but won't be consistent with the contract of the `Set` interface. – Eran May 17 '18 at 09:12

Treeset doesn't remove object - Java

1 Answers1