3
\$\begingroup\$

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<ClipHolder> associatedClips = new TreeSet<ClipHolder>();

    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;
            }
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Are the bounds the only other value in the ClipHolder? Why a TreeSet? What's it sorting on? You should be able to use a SortedMap of some sort to solve this problem, but I think we need the answers to those questions first. \$\endgroup\$
    – Eric Stein
    Commented Aug 8, 2015 at 15:59

1 Answer 1

3
\$\begingroup\$

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<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"));
        System.out.println(line);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your reply, but I am curious of a few things. Why do you say that Clip is immutable, and why did you make it an inner class of Line? Perhaps that is my fault for not being more specific in my question, but Clip is its own separate class. Also, you have introduced names for Clips. It does not particularly make sense for a clip to have a name, and is an inconvenience to the user to have to provide one. \$\endgroup\$
    – user79735
    Commented Aug 8, 2015 at 14:02
  • \$\begingroup\$ Just move the Clip to Clip.java. What comes to the "name" of a Clip: since you map a Clip to its ClipHolder, Clip must have some (at least one) immutable field, that "identifies" it whenever working with hash structures; and Clip's hashCode and equals should take that immutable into account. What happens otherwise? Well, you map one Clip to its ClipHolder, then forget about that Clip, create another Clip with same contents, and... BOOM! removeClip cannot find the ClipHolder you intend to remove. \$\endgroup\$
    – coderodde
    Commented Aug 8, 2015 at 14:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.