HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Removing an object which is wrapped in another object from a collection

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromremovingcollectionanotherwrappedwhichobject

Problem

I have Clips that can be added to or removed from any number of Lines. When I add the Clip, I put it into a ClipHolder so I can keep some bounds information with it which is specific to the Line / Clip relationship.

The trouble comes when I want to remove a Clip from the Line. I need to remove the ClipHolder in the associatedClips Set which holds the Clip, but I cannot just call associateClips.remove(Clip) (obviously as the set is holding ClipHolders, not Clips) so I am stuck iterating through the set, and searching one by one for it (see the removeClip method below.)

Within the Line class the ClipHolder class is simply an implementation detail that the user should not have to worry about. They should only have to worry about their Clips which are a separate class.

I would like to avoid burdening the users of this class with requiring them to choose a unique token to identify each instance of Clip. I think that having a reference to the Clip should be enough. (Is this reasonable?)

Does anyone have a good way of cleaning this up?

class Line {

    .....

    private class ClipHolder{
        Clip c;
        Bounds bounds;
    }

    private TreeSet associatedClips = new TreeSet();

    public void addClip(Clip c){

        ClipHolder holder = new ClipHolder();
        holder.c = c;
        calculateBounds(holder);

        associatedClips.add(holder);
    }

    public void removeClip(Clip c){

        for (ClipHolder ih: associatedClips ) {
            if(ih.c == c){
                associatedClips.remove(ih);
                break;
            }
        }
    }
}

Solution

All you need is a (hash) map for mapping a Clip to its ClipHolder. That way you can remove Clips in constant time. I have added a couple of comments directly in the code snippet:

Line.java:

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;

class Line {

    private static class Bounds {
        //...
    }

    // Clip is immutable. It is a good idea to override 'hashCode' and 'equals'
    // in order to make sure that we can safely use 'Clip's as keys in a hash
    // map.
    private static class Clip {
        private final String name;

        Clip(String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }

        @Override
        public int hashCode() {
            return name.hashCode();
        }

        @Override
        public boolean equals(Object o) {
            if (!(o instanceof Clip)) {
                return false;
            }

            return ((Clip) o).getName().equals(this.getName());
        }

        @Override
        public String toString() {
            return "[Clip: " + name + "]";
        }
    }

    // Keyword 'static' removes an implicit reference to Line from each 
    // 'ClipHolder', thus saving some space. Declare your ClipHolder a 
    // 'Comparable'.
    private static class ClipHolder implements Comparable {
        private final Clip clip;
        private final Bounds bounds;

        ClipHolder(Clip clip, Bounds bounds) {
            this.clip = clip;
            this.bounds = bounds;
        }

        @Override
        public int compareTo(ClipHolder o) {
            return this.getClip().getName().compareTo(o.getClip().getName());
        }

        public Clip getClip() {
            return clip;
        }

        public Bounds getBounds() {
            return bounds;
        }
    }

    // 1: Program to interface, not implementation.
    // 2: Since Java 7, you can use diamond inference "<>".
    private final Set associatedClips = new TreeSet<>();
    private final Map map = new HashMap<>();

    public void addClip(Clip clip) {
        Bounds bounds = null; // Compute your bounds.
        ClipHolder holder = new ClipHolder(clip, bounds);
        associatedClips.add(holder);
        // Map a clip to its holder.
        map.put(clip, holder);
    }

    public void removeClip(Clip c) {
        associatedClips.remove(map.get(c));
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder("[Line: ");

        int i = 0;

        for (ClipHolder ch : associatedClips) {
            sb.append(ch.getClip().toString());

            if (i < associatedClips.size() - 1) {
                sb.append(", ");
                ++i;
            }
        }

        return sb.append("]").toString();
    }

    public static void main(String[] args) {
        Line line = new Line();
        line.addClip(new Clip("A"));
        line.addClip(new Clip("B"));
        line.addClip(new Clip("C"));
        line.addClip(new Clip("D"));
        System.out.println(line);

        line.removeClip(new Clip("B"));
        System.out.println(line);
    }
}

Code Snippets

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;

class Line {

    private static class Bounds {
        //...
    }

    // Clip is immutable. It is a good idea to override 'hashCode' and 'equals'
    // in order to make sure that we can safely use 'Clip's as keys in a hash
    // map.
    private static class Clip {
        private final String name;

        Clip(String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }

        @Override
        public int hashCode() {
            return name.hashCode();
        }

        @Override
        public boolean equals(Object o) {
            if (!(o instanceof Clip)) {
                return false;
            }

            return ((Clip) o).getName().equals(this.getName());
        }

        @Override
        public String toString() {
            return "[Clip: " + name + "]";
        }
    }

    // Keyword 'static' removes an implicit reference to Line from each 
    // 'ClipHolder', thus saving some space. Declare your ClipHolder a 
    // 'Comparable'.
    private static class ClipHolder implements Comparable<ClipHolder> {
        private final Clip clip;
        private final Bounds bounds;

        ClipHolder(Clip clip, Bounds bounds) {
            this.clip = clip;
            this.bounds = bounds;
        }

        @Override
        public int compareTo(ClipHolder o) {
            return this.getClip().getName().compareTo(o.getClip().getName());
        }

        public Clip getClip() {
            return clip;
        }

        public Bounds getBounds() {
            return bounds;
        }
    }

    // 1: Program to interface, not implementation.
    // 2: Since Java 7, you can use diamond inference "<>".
    private final Set<ClipHolder> associatedClips = new TreeSet<>();
    private final Map<Clip, ClipHolder> map = new HashMap<>();

    public void addClip(Clip clip) {
        Bounds bounds = null; // Compute your bounds.
        ClipHolder holder = new ClipHolder(clip, bounds);
        associatedClips.add(holder);
        // Map a clip to its holder.
        map.put(clip, holder);
    }

    public void removeClip(Clip c) {
        associatedClips.remove(map.get(c));
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder("[Line: ");

        int i = 0;

        for (ClipHolder ch : associatedClips) {
            sb.append(ch.getClip().toString());

            if (i < associatedClips.size() - 1) {
                sb.append(", ");
                ++i;
            }
        }

        return sb.append("]").toString();
    }

    public static void main(String[] args) {
        Line line = new Line();
        line.addClip(new Clip("A"));
        line.addClip(new Clip("B"));
        line.addClip(new Clip("C"));
        line.addClip(new Clip("D"));
        System.out.println(line);

        line.removeClip(new Clip("B"));
    

Context

StackExchange Code Review Q#99322, answer score: 3

Revisions (0)

No revisions yet.