3
\$\begingroup\$

I am learning how to create a rest api in which I've decided to use generic for crud operation. I am attaching the code which I have written so far. Please have a look. Thanks for the review in advance.

What I expect from code review:

  1. Good part of the code (if any)
  2. Code portion which needs improvement(or any alternate way)
  3. Code which should not be there.
  4. Any source from where I can learn how to create the architecture of web-app.

Brand Model

package com.test.model;
import java.io.Serializable;

import javax.persistence.Cacheable;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.Table;

import org.hibernate.annotations.Cache;
import org.hibernate.annotations.CacheConcurrencyStrategy;


@Entity
@Cacheable
@Cache(usage=CacheConcurrencyStrategy.READ_WRITE)
@Table(name = "brand_master")
public class BrandMaster extends BaseEntity implements Serializable {

    private static final long serialVersionUID = 4045918990469712465L;

    @Id
    @GeneratedValue(strategy = GenerationType.SEQUENCE)
    @Column(name = "pk_brandId")
    private Integer id;

    @Column(name = "brand_name", unique = true)
    private String name;

    public Integer getId() {
        return id;
    }

    public void setId(Integer id) {
        this.id = id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

} 

BaseEntity:

package com.test.model;

import java.util.Date;

import javax.persistence.Column;
import javax.persistence.EntityListeners;
import javax.persistence.MappedSuperclass;

import org.springframework.data.annotation.CreatedBy;
import org.springframework.data.annotation.CreatedDate;
import org.springframework.data.annotation.LastModifiedBy;
import org.springframework.data.annotation.LastModifiedDate;
import org.springframework.data.jpa.domain.support.AuditingEntityListener;
import org.springframework.format.annotation.DateTimeFormat;

@MappedSuperclass
@EntityListeners(AuditingEntityListener.class)
public class BaseEntity {

    @CreatedDate
    @Column(name = "createdOn")
    @DateTimeFormat(pattern = "dd/MM/yyyy")
    protected Date createdOn;

    @CreatedBy
    @Column(name = "createdBy")
    protected UserMaster createdBy;

    @LastModifiedDate
    @Column(name = "updateOn")
    @DateTimeFormat(pattern = "dd/MM/yyyy")
    protected Date updatedOn;

    @LastModifiedBy
    @Column(name = "updatedBy")
    protected UserMaster updatedBy;

    public Date getCreatedOn() {
        return createdOn;
    }

    public void setCreatedOn(Date createdOn) {
        this.createdOn = createdOn;
    }

    public UserMaster getCreatedBy() {
        return createdBy;
    }

    public void setCreatedBy(UserMaster createdBy) {
        this.createdBy = createdBy;
    }

    public Date getUpdatedOn() {
        return updatedOn;
    }

    public void setUpdatedOn(Date updatedOn) {
        this.updatedOn = updatedOn;
    }

    public UserMaster getUpdatedBy() {
        return updatedBy;
    }

    public void setUpdatedBy(UserMaster updatedBy) {
        this.updatedBy = updatedBy;
    }
}

Controller

package com.test.controller;

import java.util.List;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

import com.test.dto.BrandDTO;
import com.test.service.BrandService;

@RestController
@RequestMapping(path = "**/Brand")
public class BrandController {

    private Logger log = LoggerFactory.getLogger(BrandController.class);

    @Autowired
    private BrandService BrandService;

    @GetMapping(path = "**/{id}")
    public ResponseEntity<BrandDTO> get(@PathVariable Integer id) {
        log.trace("Starting processing get request for id :" + id);

        BrandDTO Brand = BrandService.get(id);
        if (Brand != null) {
            return new ResponseEntity<BrandDTO>(Brand, HttpStatus.OK);
        }

        log.info("Entity having id not found, id : " + id);
        return new ResponseEntity<BrandDTO>(HttpStatus.NOT_FOUND);
    }

    @GetMapping
    public ResponseEntity<List<BrandDTO>> getAll(@RequestParam(value = "pageNum", required = false) Integer pageNum, @RequestParam(value = "size", required = false) Integer size) {
        log.trace("Starting processing getAll request!");

        List<BrandDTO> Brandes = BrandService.getAll(pageNum, size);
        if (Brandes != null && !Brandes.isEmpty()) {
            return new ResponseEntity<List<BrandDTO>>(Brandes, HttpStatus.OK);
        }

        log.info("No element found while hitting getAll");
        return new ResponseEntity<List<BrandDTO>>(HttpStatus.NO_CONTENT);
    }

    @PostMapping
    public ResponseEntity<BrandDTO> save(@RequestBody BrandDTO Brand) {
        log.trace("Starting processing Post request!");

        try {
            Brand = BrandService.saveOrUpdate(Brand);
        } catch (Exception err) {
            log.error("Error Occured while saving, Message : " + err.getMessage() + "; Cause :" + err.getCause());
            return new ResponseEntity<BrandDTO>(HttpStatus.INTERNAL_SERVER_ERROR);
        }
        return new ResponseEntity<BrandDTO>(Brand, HttpStatus.CREATED);
    }

    @PutMapping(path = "**/{id}")
    public ResponseEntity<BrandDTO> update(@PathVariable Integer id, @RequestBody BrandDTO Brand) {

        log.trace("Starting processing put for id :" + id);

        if (Brand != null && Brand.getId() == null) {
            log.trace("updating id in model for put request for id :" + id);
            Brand.setId(id);
        }

        if (Brand != null && Brand.getId().equals(id)) {
            log.trace("processing put request for id :" + id);

            try {
                Brand = BrandService.saveOrUpdate(Brand);
            } catch (Exception err) {
                log.error("Error Occured while saving, Message : " + err.getMessage() + "; Cause :" + err.getCause());
                return new ResponseEntity<BrandDTO>(HttpStatus.INTERNAL_SERVER_ERROR);
            }
            return new ResponseEntity<BrandDTO>(Brand, HttpStatus.OK);
        }

        log.info("Id mismatch for model (" + Brand.getId() + ") and request param :" + id);
        return new ResponseEntity<BrandDTO>(HttpStatus.NOT_MODIFIED);
    }

    @DeleteMapping(path = "**/{id}")
    public ResponseEntity<String> delete(@PathVariable Integer id) {
        log.trace("Starting  processing of delete rrequest for id :" + id);

        if (!BrandService.isExist(id)) {
            log.info("Entity not found while processing the delete request for id :" + id);
            return new ResponseEntity<>(HttpStatus.NOT_FOUND);
        }

        BrandService.delete(id);
        log.info("Entity deleted having id :" + id);
        return new ResponseEntity<>(HttpStatus.OK);
    }

}

Service:

package com.test.service;

import com.test.dto.BrandDTO;
import com.test.model.BrandMaster;

public interface BrandService extends GenericService<BrandMaster, Integer, BrandDTO> {

}

Service Impl:

package com.test.service.impl;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;

import com.test.dao.BrandDao;
import com.test.dto.BrandDTO;
import com.test.mapper.BrandMapper;
import com.test.model.BrandMaster;
import com.test.service.BrandService;

@Service("brandService")
public class BrandServiceImpl extends GenericServiceImpl<BrandMaster, Integer, BrandDTO> implements BrandService{

    @Autowired
    public BrandServiceImpl(BrandDao brandDao) {
        super(brandDao);
    }

    @Override
    public BrandMaster transformDTOToEntity(BrandDTO element) {
        return BrandMapper.INSTANCE.brandDTOToBrandMaster(element);
    }

    @Override
    public BrandDTO transformEntityToDTO(BrandMaster element) {
        return BrandMapper.INSTANCE.brandMasterToBrandDTO(element);
    }


}

Generic Service

package com.test.service;

import java.util.List;

public interface GenericService <E, I, D> {

    /**
     * E : Entity Class
     * I : type of Id element
     * D : DTO POJO
     */

    public D get(I id);
    public List<D> getAll(Integer pageNumber, Integer size);
    public D saveOrUpdate(D element);
    public void delete(I id);
    public boolean isExist(I id);
    public E transformDTOToEntity(D element);
    public D transformEntityToDTO(E element);
}

Generic Service Impl

package com.test.service.impl;

import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
import org.springframework.data.jpa.repository.JpaRepository;

import com.test.service.GenericService;

public abstract class GenericServiceImpl <E, I, D> implements GenericService<E, I, D>{

    protected JpaRepository<E, I> repository;

    private Logger log = LoggerFactory.getLogger(GenericServiceImpl.class);

    public GenericServiceImpl(JpaRepository<E, I> repository) {
        this.repository = repository;
    }

    @Override
    public D get(I id) {
        log.trace("<== Inside generic get method ==> ");

        Optional<E> element = repository.findById(id);
        if(element.isPresent()){
            log.trace("<== Got the response from dao and received an object ==>");
            return this.transformEntityToDTO(element.get());
        }

        log.info("<== No element is found for the passed id ==>");
        return null;
    }

    @Override
    public List<D> getAll(Integer pageNumber, Integer size) {
        log.trace("<== Inside getAll() of generic service ==>");


        pageNumber = pageNumber != null? pageNumber : 0;
        size = size != null? size : 10;

        Pageable page = PageRequest.of(pageNumber, size); 
        List<E> list = repository.findAll(page).getContent();

        log.trace("<== Got the response from the repository from getALL  ==>");
        return list.stream().map(entity -> transformEntityToDTO(entity)).collect(Collectors.toList());
    }

    @Override
    public D saveOrUpdate(D element) {
        log.trace("<== Inside sabeOrOpdate() of generic service ==>");

        return this.transformEntityToDTO(repository.save(transformDTOToEntity(element)));
    }

    @Override
    public void delete(I id) {
        log.trace("<== Inside delete() of generic service ==>");

        repository.deleteById(id);
    }

    @Override
    public boolean isExist(I id) {
        log.trace("<== Inside isExist() of generic service ==>");

        return repository.findById(id).isPresent();
    }

    @Override
    public E transformDTOToEntity(D element) {
        return null;
    }

    @Override
    public D transformEntityToDTO(E element) {
        return null;
    }


}

Brand DTO

package com.test.dto;

import java.io.Serializable;

public class BrandDTO implements Serializable{

    private static final long serialVersionUID = -8226355260297089645L;
    private Integer id;
    private String name;

    public Integer getId() {
        return id;
    }
    public void setId(Integer id) {
        this.id = id;
    }
    public String getName() {
        return name;
    }
    public void setName(String name) {
        this.name = name;
    }


}

Mapper

package com.test.mapper;

import org.mapstruct.Mapper;
import org.mapstruct.factory.Mappers;

import com.test.dto.BrandDTO;
import com.test.model.BrandMaster;

@Mapper
public interface BrandMapper {

    BrandMapper INSTANCE = Mappers.getMapper(BrandMapper.class);

    BrandDTO brandMasterToBrandDTO(BrandMaster brand);

    BrandMaster brandDTOToBrandMaster(BrandDTO brand);

}

Brand Dao

package com.test.dao;

import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

import com.test.model.BrandMaster;

@Repository
public interface BrandDao extends JpaRepository<BrandMaster, Integer>{

}
\$\endgroup\$
3
\$\begingroup\$

Your code is good, but some improvements could be made. Here are my suggestions:

1) Use lowercase for first letter of variable Example: BrandService BrandService; rename to: BrandService brandService;

2) Use Lombock @Getter @Setter @Slf4j @*Constructors and so on. It will make code clean and readable

3) Use ControllerAdvise for global error handling:

@ControllerAdvice
@Slf4j
public class ExceptionHandlingAdvice {
    private final Clock clock = Clock.systemDefaultZone();

    @ExceptionHandler(Exception.class)
    public ResponseEntity<ErrorResponse> handleException(Exception ex) {
        return handleException(ex.getLocalizedMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
    }

    private ResponseEntity<ErrorResponse> handleException(String message, HttpStatus httpStatus) {
        log.error("Error: " + message);
        ErrorResponse errorResponse = ErrorResponse.of(Instant.now(clock), message);
        return new ResponseEntity<>(errorResponse, httpStatus);
    }
}

4) Use dedicated errors for specific cases.

5) It is good approach to move @Transactional to service layer. If you call two dao methods on the service level they use the same transaction.

6) Can't see any reason for DTO implements Serializable.

7) Lovercase path, example: @RequestMapping(path = "**/Brand") should be **/brand

8) Consider to add content type to your controller handlers:

@PostMapping(consumes = MediaType.APPLICATION_JSON_VALUE, 
                produces = MediaType.APPLICATION_JSON_VALUE) 

9) By my opinion you use too abstract path mapping:

@RequestMapping(path = "**/Brand") -> @RequestMapping(path = "/api/brand")
@GetMapping(path = "**/{id}")` -> `@GetMapping(path = "/{id}")

10) You could use @ResponseStatus to Set HTTP Status Code and return just object instead of ResponseEntity

@PostMapping
@ResponseStatus(HttpStatus.CREATED)
public BrandDTO save(@RequestBody BrandDTO Brand) {
    ...
    return brand;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for the review. \$\endgroup\$ – Dev_Joshi Sep 24 '19 at 19:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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