Seperate out validations from a service and use custom exceptions for entities not found

Question

I am using spring-boot, spring data, and Java 6. Here is an example from a business layer to save student details.

Srvice interface:

public interface StudentService
{
    public StudentDTO save(Integer stdId, Integer divisionId, StudentDTO studentDTO);
}

Service implementation:

@Service("studentService")
public class StudentServiceImpl implements StudentSerivce
{

     @Autowire
     private StudentDAO studentDAO;


     @Autowire
     private STDDAO stdDAO;

     @Autowire
     private DivisionDAO divisionDAO;

     @Autowire
     private TSBeanMapper beanMapper;

     @Override
     public StudentDTO save(Integer stdId, Integer divisionId, StudentDTO studentDTO)
     {
         STDEntity std = stdDAO.findOne(stdId);
         if (std == null)
         {
              throw new BadRequestException("Invalid standard");
         }

         DivisionEntity division = divisionDAO.finsOne(divisionId);
         if (division == null)
         {
              throw new BadRequestException("Invalid division");
         }
         StudentEntity student = beanMapper.map(studentDTO, StudentEntity.class);
         student.setSTD(stdEntity);
         student.setDivision(divisionEntity);
         return beanMapper(studentDAO.save(studentEntity), StudentDTO.class);

     }
}

For stdId and divisionId, I have called findOne and checked against null.

I would prefer that findOne throws an exception rather that return null for my case. But in this bug report it looks like currently it's not possible.

As for those 3 lines:

1. Fetch entity
2. Check null
3. ifNull then throw an exception

These lines of code are needed in other services for stdId and divsionID.

What is the best practice to separate those validations from serviceImpl and put it in other reusable components?

Answer 1

As I do not know the other entities beside "Student" it becomes very hard to evaluate it.

One general thing: having duplicate code does not mean you have a problem. The problem comes if the duplicate code comes with real duplicate semantic for your business domain.

It may be that you are missing an business relevant abstraction of "Student" and the other entities that have std or division in common. But you should think about that if there is really a proper abstraction. You can test it by making up an artificial requirement that affects the Student and the std or division. If the other entities must change as well to have a consistent model then there is semantic duplication and you should consider an abstraction or composition.

If you have identified an abstraction to be proper then you can abstract the null checks as well. Another way is to encapsulate std and division in one object and provide a validate-method that does the check. Furthermore search the internet for refactoring methods like "introduce parameter object".

If you haven't identified an abstract responsibility then leave your code as it is. Otherwise you may fall into a type II error. (Acting if there is a problem, but there isn't). This is related to the SRP. Robert C. Martin shows this in one of his talks: https://youtu.be/Gt0M_OHKhQE?t=698. As well as you should remove duplicate responsibilities you also should separate different responsibilities.

Some other things:

1. You said you have a business layer but the separation isn't proper. And here is why: Your "business layer" is responsible to serve a web service by throwing a BadRequestException. You are mixing in technologies that belong to other layers.
2. You do not distinguish between proposal objects, consistent business objects and communication objects. They are all same (StudentDTO in, StudentDTO out).
3. The null checks are not business relevant. They are a result of mapping from one representation (String) to another representation (Std, Division) while you do not know if the source made proper assumptions of the presents of the requested business objects. This is related to the web service layer that has to ensure to pass correct non-null values to the business layer. On the other side you have code that composes an entity in the same method. That should be separated at least into another class but after all it is a different layer, maybe the business layer, in my opionion the dao layer.