current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am working on some code and I have come across the following:

private int getPosition() {
     List<IDescriptor> fragmentDescriptors = new ArrayList<IDescriptor>(mDescriptors);

     int position = -1;

     for (IDescriptor fragmentDescriptor : fragmentDescriptors) {
         if (fragmentDescriptor instanceof VideoDescriptor) {
                position = fragmentDescriptors.indexOf(fragmentDescriptor);
         }
     }

     return position;
}

In a situation like this, how can one find an index of a specific implementation within a list without using instanceOf?

Update: Few points: mDesciptors is a set. The point of the question was: The set of IDescriptor is injected into the class, the class has no idea what the set will contain, it does however know there maybe a IDescriptor that is meant for video.. the problem is the current method relies on VideoDescriptor, therefore if that implementation changes, the position will no longer be found.

I'd like a general review of this. The method was really just to demonstrate the dependency on VideoDescriptor.

share|improve this question
4  
Undeleted question to preserve the value of the answer(s). –  rolfl♦ yesterday
1  
What kind of changes are you afraid of? If the name of the class changes, how do you think you will be able to identify this (or any other relevant) class? –  Uri Agassi yesterday
2  
btw, if the mDescriptors is a Set, what's the meaning of getPosition()? A Set is not ordered... –  Uri Agassi yesterday
    
LinkedHashSet.. is ordered –  Lunar yesterday
add comment

2 Answers

up vote 4 down vote accepted

I totally agree with Uri and have some further comments:

the class has no idea what the set will contain, it does however know there maybe a IDescriptor that is meant for video..

I'm sorry, but I have to ask: If you're not sure about exactly what the Set contains, why are you storing it in a set in the first place?

Consider instead something like Map<KEY_TYPE, List<? extends IDescriptor>> descriptors; where KEY_TYPE can be a Class<? extends IDescriptor>, a String, or something else that can tell you what type it is.

When adding items to this map you could use:

List<? extends IDescriptor> list = descriptors.get(descriptor.getClass());
if (list == null) {
    list = new ArrayList<>();
    descriptors.put(descriptor.getClass(), list);
}
list.add(descriptor);

And then all you would need to do is to use descriptors.get(VideoDescriptor.class) (assuming VideoDescriptor is not subclassed, otherwise you could loop through the supertypes as well when adding to the list).

Another thought is that you can use a enum to describe what kind of IDescriptor it is, and the IDescriptor type can have a getDescriptorType method. Then you could check if (fragmentDescriptor.getDescriptorType() == DescriptorType.VIDEO)

As I only see a portion of your code and not the big picture, I can't really tell what would be best for you.

share|improve this answer
    
This is what I thought :) –  Lunar yesterday
add comment
up vote 3 down vote

Are you asking if you can find the first element in a list of a certain type without asking for its type? That's a strange question... What's wrong with instanceof?

As for the code, being code-review, I've got some observations:

This code actually finds the last instance of type VideoDescriptor in mDescriptor. It is also very inefficient, since it iterates over the collection at least three times to find it

  • It wrap it in an ArrayList,
  • It finds all items of the right type (no stop condition here - so it iterates over the whole array)
  • For each item of the correct type, it iterates over the array again using indexOf

(perhaps you are asking about indexOf and not instanceOf?)

A better way would be to simply iterate once over the collection, keeping the current position, returning upon reaching the correct type:

private int getPosition() {  
     int position = -1;

     for (IDescriptor fragmentDescriptor : mDesciptors) {
         position++;
         if (fragmentDescriptor instanceof VideoDescriptor) {
                return position;
         }
     }

     return -1;
}

if the collection has random access, you can use a classic for loop:

private int getPosition() {  
     for (int i = 0; i < mDescriptors.size(); i++) {
         if (mDescriptors.get(i) instanceof VideoDescriptor) {
                return i;
         }
     }

     return -1;
}
share|improve this answer
    
I have updated the question, as it wasn't very clear. –  Lunar yesterday
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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