1

Let's suppose that I have an interface Stuff like this:

public interface Stuff {
  Long getId();

  String getName();
}

and I have implemented this interface as StuffEntity:

@Entity
public class StuffEntity implements Stuff {
  @Id
  @GeneratedValue(strategy = GenerationType.AUTO)
  protected Long id;
  protected String name;

  // constructors, getters (implement interface methods), setters, ...
}

and I have a service interface StuffService:

public interface StuffService {
  Page<Stuff> getStuff(Pageable pageable);
}

implemented as StuffServiceImpl:

@Service
public class StuffServiceImpl implements StuffService {
  @Autowired
  private StuffEntityRepository repository;

  @Overrride
  public Page<Stuff> getStuff(Pageable pageable) {
    Page<StuffEntity> stuffEntityPage = repository.findAll(pageable);
    return new PageImpl<>(stuffEntityPage.getContent().stream()
      .map(Stuff.class::cast)
      .collect(
        Collectors.toList()), 
        stuffEntityPage.getPageable(), 
        stuffEntityPage.getTotalElements()
      );
}

I don't like casting here and creating new instance of PageImpl, so I tried something like this:

public interface StuffService {
  Page<? extends Stuff> getStuff(Pageable pageable);
}
@Service
public class StuffServiceImpl implements StuffService {
  @Autowired
  private StuffEntityRepository repository;

  @Overrride
  public Page<? extends Stuff> getStuff(Pageable pageable) {
    return repository.findAll(pageable);
}

As I've already said, I don't like casting and creating new PageImpl instance in my service class, but I like how my code looks cleaner that way. The other approach does not need casting or creating new PageImpl instance in my service class, but I am little worried about service's client side, because now I return wildcard instead of an interface.

Which approach do you think is better?

Titulum
  • 9,928
  • 11
  • 41
  • 79
krki
  • 11
  • 2
  • "I am little worried about service's client side" Why? 2nd one is objectively better. – Michael Sep 14 '20 at 11:22
  • I presume that Page is [Spring's version](https://docs.spring.io/spring-data/commons/docs/current/api/org/springframework/data/domain/Page.html)? Or if not, it's at least read-only i.e. you cannot add items to a page? In which case, [PECS](https://stackoverflow.com/questions/2723397/what-is-pecs-producer-extends-consumer-super) applies. It's a producer, so `? extends` is the correct choice. – Michael Sep 14 '20 at 11:24
  • I am worried about client's side because now it has to handle wildcard. But yes, returned `Page` is supposed to be read-only. Also, it will never be a collection of multiple types of `Stuff`, but I might have `public class StuffOtherImpl implements Stuff` and `public class StuffServiceOtherImpl implements StuffService` which will return `Page` of `StuffOtherImpl` . – krki Sep 14 '20 at 11:32
  • 1
    The real problem is that you are defining interfaces for your entities, this is generally something to avoid. – M. Deinum Sep 14 '20 at 11:46
  • @M.Deinum beeeecause...? – Michael Sep 14 '20 at 11:48
  • I noted the same thing. This is using an interface to "mask" an entity instead of properly transforming the entity to a DTO with the required data. Difference being that with DTOs the entities never escape the DAL, while here you're putting a coat on it and saying "nobody will notice". Lose the interface, return entities and map them to DTOs (e.g. in façade) and *this* particular problem will disappear (and you'll have a more robust architecture). – Kayaman Sep 14 '20 at 12:03

1 Answers1

0

It is always good practice to not use wildcard type as return type.

It is highly recommended not to use wildcard types as return types. Because the type inference rules are fairly complex it is unlikely the user of that API will know how to use it correctly.

Let's take the example of method returning a "List<? extends Animal>". Is it possible on this list to add a Dog, a Cat, ... we simply don't know. And neither does the compiler, which is why it will not allow such a direct use. The use of wildcard types should be limited to method parameters.

This rule raises an issue when a method returns a wildcard type.

Source

Michael
  • 41,989
  • 11
  • 82
  • 128
  • Link only answers aren't good answers, and charity upvotes aren't good upvotes. – Kayaman Sep 14 '20 at 11:46
  • 1
    @Kayaman Telling people how to use their votes is waste of time and energy, people will do what they want – Michael Sep 14 '20 at 11:56
  • 1
    Disagree with this answer, even though it's from an "authoritative" source like Sonar. The whole justification seems to be that you cannot add to a list. Well, what if it's not a Collection? OP has a Page, and it's not possible to add to one. Seems like PECS applies. I disagree that it's "*unlikely the user of that API will know how to use it correctly*" – Michael Sep 14 '20 at 11:58