12

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?

10
  • 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? Nov 29, 2014 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, 2014 at 20:12
  • @9000 Unfortunately no, and I believe that adding such a thing would clutter it a little further. Nov 29, 2014 at 23:42
  • 8
    Is there a reason that SpecificItem's implementation doesn't just call also() first then? Nov 29, 2014 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, 2016 at 7:38

6 Answers 6

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.

2
  • 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. Nov 17, 2015 at 8:35
  • More functionally: stuff.stream().filter(SpecificItem.class::isIstance).map(SpecificItem.class::cast).forEach(SpecificItem::also);
    – strickli
    Mar 25, 2021 at 1:29
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.

4
  • 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. Nov 29, 2014 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. Nov 29, 2014 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. Nov 30, 2014 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. Nov 30, 2014 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.

1
  • That's quite sensible, and I may end up eventually refactoring to do that. Dec 2, 2014 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
  • 4
    That is not the way: to improve bad design by making it worse. Nov 29, 2014 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. Nov 29, 2014 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." Nov 29, 2014 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.

2
  • 2
    Isn't this just a less readable version of the original? Apr 2, 2015 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, 2015 at 23:59

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