8

I have an unexpected situation in a project in which all types extending one class are packed into a Java collection; but only a specific extension of that class contains an additional method. Let's call it "also()"; and let me be clear that no other extension has it. Right before performing one task on every item in that collection, I need to call also() on every item that implements it.

The easiest way forward is this:

stuff.stream().filter(item -> item instanceof SpecificItem)
            .forEach(item -> ((SpecificItem)item).also()));
stuff.stream().forEach(item -> item.method());

It works fine, but I'm not comfortable with the "instanceof" in there. That's generally a marker of bad code smell. It is very possible that I will refactor this class just to get rid of it. Before I do something that dramatic, though, I thought I would check with the community and see if someone with more experience with either Streams or Collections had a simpler solution.

As an example (certainly nonexclusive), is it possible to get a view of a collection that filters entries by class?

  • 2
    If you need to filter on SpecificItem, and this is the proper way to specify the predicate, then why are you squeamish about the instanceof being there? – Robert Harvey Nov 29 '14 at 20:10
  • Do the objects in the stream implement some common interface that would let you to discriminate between classes? Some .getKind() or .isSpecific()? – 9000 Nov 29 '14 at 20:12
  • @9000 Unfortunately no, and I believe that adding such a thing would clutter it a little further. – Michael Eric Oberlin Nov 29 '14 at 23:42
  • 8
    Is there a reason that SpecificItem's implementation doesn't just call also() first then? – Tristan Burnside Nov 29 '14 at 23:46
  • 1
    @LuísGuilherme Iterable doesn't have a stream() method, but you can call forEach() directly on an Iterable. – shmosel Aug 23 '16 at 7:38
3

I'd suggest tossing in a .map call to do the cast for you. Then your later code can use the 'real thing'.

Before:

stuff.stream().filter(item -> item instanceof SpecificItem)
            .forEach(item -> ((SpecificItem)item).also()));

After:

stuff.stream().filter(item -> item instanceof SpecificItem)
            .map(item -> (SpecificItem)item)
            .forEach(specificItem -> specificItem.also()));

This isn't perfect, but seems to clean things up a little.

  • It is my interest to remove as much casting as I can from the work code, and you're right, map meant for that exactly. – Michael Eric Oberlin Nov 17 '15 at 8:35
3

I have an unexpected situation in a project in which all types extending one class are packed into a Java collection; but only an extension of that class implements a method. Let's call it "also()". Right before performing one task on every item in that collection, I need to call also() on every item that implements it.

That is obviously a defective design. From what you wrote it is not clear, what it means, that one class does not implement the method. If it simply does nothing, it wouldn't matter, so I assume there is an unwanted side effect.

It works fine, but I'm not comfortable with the "instanceof" in there.

Your guts are right. Good object oriented design would work without further knowledge what exactly an object, resp whether it is an instance of a special kind.

Before I do something that dramatic, though, I thought I would check with the community and see if someone with more experience with either Streams or Collections had a simpler solution.

Refactoring is not dramatic. It improves code quality.

With your given codebase, the simplest solution would be, to make sure, you have two separate collections, one with the parent and one with the child class. But that's not quite clean.

  • I've edited my question to be more specific. To clarify further, this is a crack-job for a change needed immediately; an extension of the class has to be taken into account. As to refactoring, let's just say that it's an essential part of being a programmer. However, I'm looking for the least dramatic refactor possible. – Michael Eric Oberlin Nov 29 '14 at 23:39
  • To your question, there is a clear answer: if the filter needs to know, with which instance it has to deal, there is no way around instance of (except some kind of weirdo reflection-magic, which is a more cryptic way of asking instanceof). But the time you save now for a quick and dirty solution is spent later on in the product lifecyle double on fixing bugs due to a bad design. – Thomas Junk Nov 29 '14 at 23:58
  • @MichaelEricOberlin: So if I hear Thomas correctly, your choices appear to be: change the design, or leave it the way it is. – Robert Harvey Nov 30 '14 at 1:12
  • @RobertHarvey put it this way, it sounds tautologic ;) if he do es not want to refactor, there is no other option left than going down the current road. – Thomas Junk Nov 30 '14 at 1:36
2

It's hard to give specific recommendations without knowing what SpecificItem and also() actually are, but:

Define also() on the superclass of SpecificItem. Give it a default implementation which does nothing (give it an empty method body). Individual subclasses can override it if desired. Depending on what also actually is, you may need to rename it to something which makes sense for all the classes involved.

  • That's quite sensible, and I may end up eventually refactoring to do that. – Michael Eric Oberlin Dec 2 '14 at 22:05
1

an alternative:

  1. make SpecificItem implement an interface (say "Filterable")
  2. make a new class that extends Stream
  3. create a new 'filter' method which accept object that implement your interface
  4. override the original stream method and redirect it to your implementation (by casting the predicate parameter to your interface)

that way only objects which implement your interface would be able to pass themeselves to your method... no need to use instanceof

public class MyStream extends Stream
{
    //...

    Stream<Filterable> filter(Predicate<? implements Filterable> predicate)
    {
         return super.filter(predicate);
    }
}
  • 3
    That is not the way: to improve bad design by making it worse. – Thomas Junk Nov 29 '14 at 21:47
  • I have to agree with Thomas Junk here. Step three basically does what I'm already doing; and without again using instanceof, I fail to see how your method would resolve this. – Michael Eric Oberlin Nov 29 '14 at 23:35
  • 1
    @MichaelEricOberlin: This is exactly what I was worried about; creating a Rube Goldberg just to satisfy someone's idea of a "best practice." – Robert Harvey Nov 29 '14 at 23:52
1

I hope I'm not missing anything obvious here (also as suggested by @Tristan Burnside's comment), but why can't SpecificItem.method() call also() first?

public class SpecificItem extends Item {
    ...
    public void method() {
        also();
        super.method();
    }
}

As an example (certainly nonexclusive), is it possible to get a view of a collection that filters entries by class?

A stream-y way I can think of, at the expense of maybe some performance impact( YMMV), is to collect() via Collectors.groupingBy() on the class as the key, then pick what you want from the resulting Map. The values are stored as a List, so if you were expecting to do such a filtering on a Set and you hope to get a filtered Set out of it, then you will need an additional step to further put the values into a resulting Set as well.

-1

Here is my approach:

stuff.stream().filter(item -> SpecificItem.class.isInstance(item)).map(item  -> SpecificItem.class.cast(item)).forEach(item -> item.also());
stuff.stream().forEach(item -> item.method());

No instanceof, no explicit casts (actually it is an explicit cast, but masked by a method call). I think this is as good as it can get.

  • 1
    Isn't this just a less readable version of the original? – grahamparks Apr 2 '15 at 23:05
  • Well, it actually does the same in a typesafe way. It may be simpler depending on the context of the code. I use this for filtering certain types inside a generic list, except for the forEach part. – beto Apr 2 '15 at 23:59

protected by user40980 Apr 3 '15 at 2:19

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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