7

Question is a bit rhetorical. I've had an argument with my colleagues regarding the following pattern:

@Component
public class MetaService {

    public static UserService userService;
    public static GroupService groupService;   
    public static PermissionService permissionService;            
    // ...more fields

    @Autowired
    public MetaService(UserService userService,
                       GroupService groupService
                       PermissionService permissionService) {

        MetaService.userService = userService;
        MetaService.groupService = groupService;
        MetaService.permissionService = permissionService;           
    }
}

Basically, MetaService is an entry point/wrapper for multiple Spring beans @Service classes. And there's one more similar MetaDao wrapper for @Repository beans class:

@Component
public class MetaDao {

    public static UserDao userDao;
    public static GroupDao groupDao;
    public static PermissionDao permissionDao;
    // ...more fields   

    @Autowired
    public MetaDao(UserDao userDao,
                   GroupDao groupDao,
                   PermissionDao permissionDao) {

        MetaDao.userDao = userDao;
        MetaDao.groupDao = groupDao;
        MetaDao.permissionDao = permissionDao;
    }
}

The idea of having such a Meta-class is to reduce code base within @RestController and @Service classes, by providing concise API to call @Service and @Repository methods, for example:

@RestController
public class UserController {

    @PostMapping
    public UserCreateResponse createUser(UserCreateRequest request) {
        MetaService.userService.createAndReturn(userEntity);
        // other service calls
    }
}

In other words, this is an alternative to having @Autowired fields/constructors for each @Service within each @Controller etc. In my opinion this approach greatly reduces boilerplate repetitive code a lot.

Well, my colleagues are saying that: such an approach makes the code harder to understand/maintain since combining @Service components in one place hides dependency model of other components which use them. In other words, if there were fields declared for UserController class. It would've been easier to quickly find out which components UserController depends upon. In addition, it's harder to mock such dependencies doing unit testing. And the most important that such approach is super-anti-pattern. Whereas I can agree with mocking part, everything else seems ridiculous to me.

Question is, is this truly an anti-pattern in any way?

Nikolas Charalambidis
  • 40,893
  • 16
  • 117
  • 183
Mikhail Kholodkov
  • 23,642
  • 17
  • 61
  • 78
  • 2
    I kinda agree with your colleagues at it obfuscates code and make it really harder to understand. Moreover, what is the benefit of having such MetaClasses? In addition, I'd say that without these MetaClasses, you have a better capacity to ensure that Controllers can use only what they need to – DamCx Jun 05 '18 at 12:19
  • 2
    Agreeing to @DamCx. Btw. I wouldn't recommend to have the service references in static variables. Consider the example that you have 2 application contexts in the same VM that have both the meta service bean but with different implementations of the other services. – Steffen Harbich Jun 05 '18 at 12:26
  • 1
    For the close-voters: Detecting anti-pattern is really not "primarily opinion-based". – Nikolas Charalambidis Jun 05 '18 at 12:34
  • 1
    You are trying to introduce a Service Locator on top of your constructor injection + dependency injection, and Service Locator is generally recommended *against*, because it hides dependencies. – EpicPandaForce Jun 05 '18 at 12:37
  • 2
    The real antipattern seems to be the fact that nothing is isolated anymore. As your MetaClasses will grow, people will be able to abuse more and more of it, as it exposes classes that are not needed, and may in fact end with code duplication as every service will be able to reproduce other services functionalities. They'll be harder to debug, as a service is no longer limited to its specific DAOs but may be accessing anything. It'll be impossible to know the real dependencies between your components. The Mock issue is also a no-go, and you get no benefit for doing this. – Christophe Douy Jun 05 '18 at 12:39

3 Answers3

4

With regard to the principles of OOP, I find exposing grouped services through public static attributes even with final keyword as a bad principle. I think this approach increases the complexity: What if more of the DAOs/services are about to be added? - Then you'll be forced to enlarge the MetaDao and MetaSerice classes.

Exposing these services through this facade, another unnecessary layer would allow the UserController use and touch services which really doesn't need - Is it necessary? It breaks the encapsulation. Use this widely used approach:

@RestController
public class UserController {

    @Autowired
    private final UserService userService;

    // ...
}
Nikolas Charalambidis
  • 40,893
  • 16
  • 117
  • 183
4

This is clearly an anti-pattern to me.

  1. Why is there even a need for this? The UserController needs the UserService, why would it include a MetaService containing a GroupService it doesn't need?

  2. Not sure what your MetaDAO is supposed to do, bundle together objects that are in a certain relation? I highly doubt that's a proper implementation for most objects (can a user be in many groups? can he have several permissions?) What domain relevance is this supposed to serve? Or are they supposed to be @Repositorys?

  3. The static members are public static, but not final - breach of encapsulation. It's basically a badly implemented Singleton pattern.

  4. The MetaService might as well implement all three interfaces and maintain a relation to the repositories. No real need to put two layers of services between controller and repository if the actual domain is properly represented by a single service.

Also, I think this question belongs in https://softwareengineering.stackexchange.com.

daniu
  • 14,137
  • 4
  • 32
  • 53
  • 1. The need is to simply reduce code base without declaring multiple common fields in each controller/service. 2. MetaDao is just another wrapper for DAO classes, which is used within @Service (s). Idea is the same. 3. Agree. It can be easily changed to static final though. Will require ugly capital case though. 4. Agree. But it would require multiple such Meta classes to be created. Thanks for answer! – Mikhail Kholodkov Jun 05 '18 at 12:49
  • And yes, it probably belongs to https://softwareengineering.stackexchange.com. But since it's kinda bounded to Java/Spring, I though it'd be better to ask here. – Mikhail Kholodkov Jun 05 '18 at 12:56
1

You're introducing Service Locator called MetaService that all other services depend on, instead of directly depending on their dependencies. This also means that you introduce hidden dependencies, see here:

MetaService.userService.createAndReturn(userEntity);
           _           _
          /|\         /|\
           |           |

You no longer know based on your constructor arguments what your dependencies really are.

If you already have a dependency injection system in place, you generally shouldn't need a service locator.

EpicPandaForce
  • 79,669
  • 27
  • 256
  • 428