1
\$\begingroup\$

I used factory design pattern to identify the service according to the enum that comes from the api uri as request param. Everything seems okay according to me but cannot be sure if I used it right.

@RestController
@RequestMapping(GA)
public class GaController {

    @Autowired
    private GaServiceFactory factory;

    @PostMapping(value = INITIALIZE, produces = APPLICATION_JSON_VALUE)
    public ResponseEntity<GaDTO> initialize(@RequestParam GaType gaType) {
        return status(CREATED).body(factory.getInstance(gaType).initialize());
    }

    @GetMapping(value = FIND, produces = APPLICATION_JSON_VALUE)
    public ResponseEntity<GaDTO> find(@PathVariable String id, @RequestParam GaType gaType) {
        return status(FOUND).body(factory.getInstance(gaType).find(id));
    }

    @GetMapping(value = FIND_ALL, produces = APPLICATION_JSON_VALUE)
    public ResponseEntity<List<GaDTO>> findAll(@RequestParam GaType gaType) {
        return status(FOUND).body(factory.getInstance(gaType).findAll());
    }

This is my factory;

@Service
public class GaServiceFactory {

    @Autowired
    private ApplicationContext appContext;

    public IGaService getInstance(GaType type) {

        if (MAN.equals(type)) {
            return appContext.getBean(ManGaService.class);
        }

        throw new RuntimeException("No matching service could be created !");
    }

}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Vogel612
    Jan 5, 2022 at 14:45

2 Answers 2

1
\$\begingroup\$

Welcome to CodeReview.

By the code you have shown, using the factory pattern may be overengineering. Unless you plan on adding more services, just use the ManGaService directly (to have less coupling, you could declare the variable of type IGaService).

But maybe, and more importantly, why would you have the desired service be specified as a query parameter? Doesn't it make more sense to define separate resources and URLs?

If you simply want to unify the way operations are handled for all resources, then you could make the GaType parameter a PathVariable, so that it is transparent to the user of your API.

For example, this could be a controller for the respurce handled by the ManGaService:

@RestController
@RequestMapping("/path/of/ga/man")
public class ManGaController {

  // Using constructor autowiring allows to declare fields as final
  private final IGaService manService;

  @Autowired
  private ManGaController(ManGaService manService) {
    this.manService = manService;
  }

  @PostMapping
  public ResponseEntity<GaDTO> initialize() {
    // I prefer class imports instead of static method imports
    return ResponseEntity.status(HttpStatus.CREATED)
      .body(manService.initialize());
  }

  ...
}

Then, there would be some other controllers for the other existing resources handled by IGaServices.

Lastly, as a comment, instead of creating a RuntimeException directly, either use a specialized subclass, or declare your own. If you were to keep using your factory pattern approach, IllegalArgumentException would be more suitable

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the answer! Could you please explain more or give an example about defining seperate resources and URLs? Because me either was not sure how to get the gaType to define the service. It is enum but I couldn't find another way to define which service to serve. \$\endgroup\$ Jan 5, 2022 at 13:46
  • \$\begingroup\$ By the way, yes I'm planning to add more services. I have one service as of now but it will increase. \$\endgroup\$ Jan 5, 2022 at 14:02
  • \$\begingroup\$ @abdumuhammadal I have edited my answer to show an example of a Man resource controller \$\endgroup\$ Jan 5, 2022 at 14:14
  • \$\begingroup\$ Thank you! And last question; if I want to keep factory design pattern, is using request param to get gaType the only mistake, ignoring overengineering? I mean, does it seem to you okay about using factory design pattern in a right way? \$\endgroup\$ Jan 5, 2022 at 14:28
  • 1
    \$\begingroup\$ @abdumuhammadal Regarding the current use of the factory pattern, I would throw a specialized subclass of RuntimeException, such as IllegalArgumentException. Imagine you were trying to catch the exception to handle it. Currently, you'd have to catch a RuntimeException, which would lead to possibly hiding other errors (such as NullPointerExceptions). Regarding code style, I would change the use of a simple if-else structure in the getInstance method to a switch expression or a simple switch statement. \$\endgroup\$ Jan 6, 2022 at 8:44
2
\$\begingroup\$

If you are free to change the request mappings, you should make the distinguishion between "manga" and "conga" in a more restful manner by using the @PathVariable annotation:

@GetMapping("/my/favourite/{gaType}/ga")
public ResponseEntity<List<GaDTO>> findAll(@PathVariable String gaType) {
    ...
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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