Skip to main content

The service should focus on the business logic and should not be concerned with HTTP status codes, JSON formats, etc... It should be possible to use the service from a controller, but also be driven from a CLI instead of aan HTTP endpoint without change.

With athe HTTP status code of 400.

If you want to expose the expiry date as a field in the JSON response, then you can create a method or a field in the exception and annotate it with @ResponseErrorProperty

The service should focus on the business logic and should not be concerned with HTTP status codes, JSON formats, etc... It should be possible to use the service from a controller, but also be driven from a CLI instead of a HTTP endpoint without change.

With a HTTP status code of 400.

If you want to expose the expiry date as a field in the JSON response, you can create a method or field in the exception and annotate it with @ResponseErrorProperty

The service should focus on the business logic and should not be concerned with HTTP status codes, JSON formats, etc... It should be possible to use the service from a controller, but also be driven from a CLI instead of an HTTP endpoint without change.

With the HTTP status code of 400.

If you want to expose the expiry date as a field in the JSON response, then you can create a method or a field in the exception and annotate it with @ResponseErrorProperty

Source Link

A controller and a service are separate things for a reason: They focus on different things.

The service should focus on the business logic and should not be concerned with HTTP status codes, JSON formats, etc... It should be possible to use the service from a controller, but also be driven from a CLI instead of a HTTP endpoint without change.

A controller should focus on the HTTP stuff: status codes, JSON response format, request validation, etc...

Taking your example, I would rewrite the service method like this:

@Transactional
public Voucher claimVoucher(Long userId, Long templateId) {
    VoucherTemplate template = templateRepository.findById(templateId)
            .orElseThrow(() -> new VoucherTemplateNotFoundException(templateId));

    LocalDateTime now = TimeUtils.now();

    if (!template.isActive() || template.isArchived())
        throw new VoucherTemplateInActiveException(templateId);

    if (template.getValidFrom().isAfter(now))
        throw new VoucherTemplateNotActiveYetException(templateId);

    if (template.getValidTo().isBefore(now))
        throw new VoucherTemplateExpiredException(templateId);

    if (template.getClaimedCount() >= template.getTotalQuantity())
        throw new VoucherTemplateOutOfStockException(templateId);

    if (voucherRepository.existsByUserIdAndTemplate_Id(userId, templateId))
        throw new VoucherTemplateAlreadyClaimedException(templateId, userId);

    try {
        template.setClaimedCount(template.getClaimedCount() + 1);
        templateRepository.saveAndFlush(template);

        Voucher voucher = Voucher.builder()
                .template(template)
                .userId(userId)
                .active(true)
                .used(false)
                .claimedAt(now)
                .validFrom(template.getValidFrom())
                .validTo(template.getValidTo())
                .build();

        Voucher saved = voucherRepository.save(voucher);
        return saved;

    } catch (DataIntegrityViolationException e) {
        throw new VoucherTemplateAlreadyClaimedException(templateId, userId);
    } catch (OptimisticLockException e) {
        throw new VoucherTemplateException(e);
    }
}

Some things to note:

  • The method returns the Voucher object, not a DTO.
  • The method throws dedicated business exceptions when there is a problem.

If you change the service like that, then the controller becomes:

@PostMapping("/claim/{templateId}")
@PreAuthorize("hasRole('USER')")
public VoucherDto claimVoucher(
        @AuthenticationPrincipal CustomUserDetails user,
        @PathVariable("templateId") Long templateId
) {
    return VoucherDto.of(voucherService.claimVoucher(user.getId(), templateId));
}

For this to work, you need to create a VoucherDto.of(Voucher) method that does the conversion from the Voucher domain object to the VoucherDto web layer object. Personally, I would rename VoucherDto to VoucherResponse probably, but that is entirely optional.

For the error cases, I use the error-handling-spring-boot-starter (Note: I am the author of the library) which will automatically translate the exception into a nice JSON error.

For example, take this exception:

@ResponseStatus(HttpStatus.BAD_REQUEST)
public class VoucherTemplateExpiredException extends RuntimeException {

  public VoucherTemplateExpiredException(String templateId) {
    super("The voucher template %s is expired.".formatted(templateId));
  }
}

This would return this JSON (by default, you can customize this if needed):

{
  "code": "VOUCHER_TEMPLATE_EXPIRED",
  "message": "The voucher template 123 is expired."
}

With a HTTP status code of 400.

If you want to expose the expiry date as a field in the JSON response, you can create a method or field in the exception and annotate it with @ResponseErrorProperty

You can read more about how I do this layering in my blog entry How I write production-ready Spring Boot applications.