Spring Boot RESTful service

Question

I have developed this restful service using spring boot. Please look at the code if there is something to change to be more consistently.

Resource

@RestController
@RequestMapping(value = "/api/cars")
public class MuscleCarResource {
 @Autowired
 private MuscleCarService muscleCarService;
 @RequestMapping(value = "/get-car/{id}", method = RequestMethod.GET)
 public ResponseEntity<MuscleCar> getMuscleCar(@PathVariable("id") int id) {
 try {
 MuscleCar muscleCar = muscleCarService.getCar(id);
 if (muscleCar != null) {
 return ResponseEntity.status(HttpStatus.OK).body(muscleCar);
 } else {
 return ResponseEntity.status(HttpStatus.NOT_FOUND).build();
 }
 } catch (Exception e) {
 return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
 }
 }
 @RequestMapping(value = "/delete-car/{id}", method = RequestMethod.DELETE)
 public ResponseEntity<Void> deleteMuscleCar(@PathVariable("id") int id) {
 try {
 muscleCarService.removeCarFromList(id);
 return (ResponseEntity<Void>) ResponseEntity.status(HttpStatus.OK);
 } catch (Exception e) {
 return (ResponseEntity<Void>) ResponseEntity.status(HttpStatus.BAD_REQUEST);
 }
 }
 @RequestMapping(value = "/add-car", method = RequestMethod.POST)
 public ResponseEntity<Void> addCarToList( @RequestBody MuscleCar muscleCar) {
 try {
 muscleCarService.addCarToList(muscleCar);
 return ResponseEntity.status(HttpStatus.OK).build();
 } catch (Exception e) {
 e.printStackTrace();
 return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
 }
 }
 @RequestMapping(value = "/cars", method = RequestMethod.GET)
 public ResponseEntity<List<Map<String, Object>>> listAllCars() {
 try {
 List<Map<String, Object>> result = muscleCarService.listAllCars();
 return ResponseEntity.status(HttpStatus.OK).body(result);
 } catch (Exception e) {
 return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
 }
 }
 @RequestMapping(value = "/update-car/{id}", method = RequestMethod.PUT)
 public ResponseEntity<Void> updateCar(@PathVariable("id") int id, @RequestBody MuscleCar muscleCar) {
 try {
 muscleCarService.updateCarFromList(id, muscleCar);
 return ResponseEntity.status(HttpStatus.OK).build();
 } catch(Exception e ) {
 e.printStackTrace();
 return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
 }
 }
}

Service

@Service
public class MuscleCarService {
 @Autowired
 private MuscleCarDao muscleCarDao;
 public MuscleCar getCar(int id) {
 if (id <= 0) {
 throw new IllegalArgumentException("ID can not be 0 or <0");
 }
 return muscleCarDao.getCarFromList(id);
 }
 public void removeCarFromList(int id) {
 if (id <= 0) {
 throw new IllegalArgumentException("ID can not be 0 or <0 or this id do not exist");
 }
 muscleCarDao.removeCarFromList(id);
 }
 public List<Map<String, Object>> listAllCars() {
 List<Map<String, Object>> result = muscleCarDao.listAllCars();
 if (result.size() > 0) {
 return result;
 } else {
 return null;
 }
 }
 public void addCarToList(MuscleCar muscleCar) {
 if (muscleCar == null) {
 throw new IllegalArgumentException("The passed object cna not be null.");
 }
 muscleCarDao.addCarToList(muscleCar);
 }
 public void updateCarFromList(int id, MuscleCar muscleCar) {
 if ( id <= 0 && muscleCar == null) {
 throw new IllegalArgumentException("The passed object cna not be null.");
 }
 muscleCarDao.updateCarFromList(id, muscleCar);
 }
}

DAO

@Repository
public class MuscleCarDaoImpl extends JdbcDaoSupport implements MuscleCarDao {
 @Autowired
 public MuscleCarDaoImpl(JdbcTemplate jdbcTemplate, DataSource dataSource) {
 jdbcTemplate = new JdbcTemplate(dataSource);
 setJdbcTemplate(jdbcTemplate);
 }
 @Override
 public MuscleCar getCarFromList(int id) {
 String sql = "select * from muscle_cars where id = ?";
 Object[] args = new Object[] { id };
 return getJdbcTemplate().queryForObject(sql, args, new MuscleCarRowMapper());
 }
 @Override
 public void removeCarFromList(int id) {
 String sql = "delete from muscle_cars where id = ?";
 Object[] args = new Object[] { id };
 getJdbcTemplate().update(sql, args);
 }
 @Override
 public void addCarToList(MuscleCar muscleCar) {
 String sql = "insert into muscle_cars (car_brand, car_model, horsepower, car_engine) values (?, ?, ?, ?)";
 Object[] args = new Object[] {muscleCar.getCarBrand(), muscleCar.getCarModel(), muscleCar.getHorsepower(), muscleCar.getCarEngine()};
 getJdbcTemplate().update(sql, args);
 }
 @Override
 public void updateCarFromList(int id, MuscleCar muscleCar) {
 String sql = "update muscle_cars set car_brand = ?, car_model = ?, horsepower = ?, car_engine = ? where id =?";
 Object[] args = new Object[] {muscleCar.getCarBrand(), muscleCar.getCarModel(), muscleCar.getHorsepower(), muscleCar.getCarEngine(), id};
 getJdbcTemplate().update(sql, args);
 }
 @Override
 public List<Map<String, Object>> listAllCars() {
 String sql = "select * from muscle_cars";
 return getJdbcTemplate().queryForList(sql);
 }
}

• gervais.b: Please. You cannot seriously name it "restful" when you have verbs in your resources path.

Answer 1

Catching stricter exceptions

try {
 // ..
} catch (Exception e) {
 // ...
}

You can be stricter in catching the Exception you want, e.g. a SQLException. This makes the code clearer to comprehend, especially in terms of understanding exception handling.

Named parameterized bindings

JdbcTemplate supports the use of the named parameterized bindings, which can make the SQL statements slightly easier to read, and more defensive against any accidental misplacements of variables.

null vs empty Collection

It's generally recommended to return an empty Collection instead of a null, so that callers of the method can still expect an object to call upon, e.g. list.isEmpty().

On a related note, your service's listAllCars() method can use result.isEmpty() instead of result.size() > 0 to check if the returned collection is empty or not.

Spelling typo

cna is spelled wrongly twice, due to a copy-paste. :)

Answer 2

Should be changed:

• Request mappings are not that RESTful due to the verbs:
  • POST /api/cars/add-car => POST /api/cars
  • GET /api/cars/cars => GET /api/cars
  • GET /api/cars/get-car/{id} => GET /api/cars/{id}
  • PUT /api/cars/update-car/{id} => PUT /api/cars/{id}
  • DELETE /api/cars/delete-car/{id} => DELETE /api/cars/{id}
• POST can return HTTP 201 Created.
• deleteMuscleCar causes unchecked warnings due to the generic casts. You can suppress the warnings for the whole method with @SupressWarnings("unchecked") since it's short (however, I would extract local variables and suppress warnings for the variables narrowing down the scope for the @SuppressWarnings annotation).
• There are methods named remove*. They must be named delete*. See the difference here.
• You have missed ex.printStackTrace(); for several controller methods.
• HTTP 400 Bad Request may be not a subject to be logged (it's a too common error).
• POST /api/cars/add-car only parameter is annotated with @RequestBody and the body is required by default, so the service "create" method may have the null check removed.
• POST /api/cars/add-car should return a created entity.
• GET /api/cars/cars cannot return HTTP 400 Bad Request.
• PUT /api/cars/update-car/{id} should return a modified entity.
• DELETE /api/cars/delete-car/{id} should return HTTP 204 No Content.
• You can reduce the amount of code in your controllers when checking for errors using @ControllerAdvice-annotated classes. This allows you to define default controller response status and not create response entities yourself delegating this job to Spring Framework. @ResponseStatus is used to set default response statuses.
• Having no results must return an empty collection, never null. Note how queryForList works.
• Technically, IDs can be negative.
• MuscleCarDaoImpl dependencies can be changed: you can omit DataSource and configure it in a more appropriate place like configuration files.

Depends on code conventions, style and preferences:

• Java annotations value can be omitted if it's a single annotation argument. For example, @RequestMapping("/api/cars")
• Java static imports can improve readability. For example, return status(OK).body(result);
• Controller methods parameters tend to have annotations making lines of code too lengthy. You can put a single annotated method parameter per line.
• Your controller is a CRUD-controller: Create, Read, Update and Delete. Maybe it's better to rearrange the controller methods as addCarToList, listAllCars, getMuscleCar, updateCar, deleteMuscleCar?
• You can make the controller dependencies final and create an @AutoWired constructor.
• I would not use public modifier as much as possible. Spring Framework is smart enough to find not public components.
• MuscleCarService interface might be introduced in order to have loose coupling.
• Collection.size() > 0 can be replaced with !Collection.isEmpty().
• Throwing business exceptions could be more preferable rather than returning special values. Say, NoSuchElementException may be thrown when no entity found by ID, and then caught in the controller exceptions advice.
• final Object[] args = new Object[] { foo, bar, baz } can be replaced with shorter final Object[] args = { foo, bar, baz }.
• queryForObject mapper and args can be swapped in favor of varargs methods (variadic parameters). update is also variadic.
• Note that MuscleCarRowMapper can be a singleton: it has no state thus should not be instantiated in every update. Also its constructor can be private (we instantiate it itself, we know better) and the static accessor can even weaken the return type (what if you need to replace/wrap an instance of another concrete type?). Nevertheless, this mapper is too specific (a very special mapper for 4 columns) and can be even made static+private in the DAO class (can be anonymous class, but I think that it can be a method letting Java 8 compiler to align the mapper method with the RowMapper interface with a method reference).

My personal preferences:

• I would name interface with starting with I. Say, I would rename Foo and FooImpl to IFoo and Foo respectively. In my opinion, *Impl looks worse than I*.
• I would prefer UPPER_CASE for SQL statements.

Representation layer

@RestController
@RequestMapping("/api/cars")
final class MuscleCarController {
 private final IMuscleCarService muscleCarService;
 @Autowired
 MuscleCarController(
 final IMuscleCarService muscleCarService
 ) {
 this.muscleCarService = muscleCarService;
 }
 @RequestMapping(method = POST)
 @ResponseStatus(CREATED)
 MuscleCar create(
 @RequestBody final MuscleCar muscleCar
 ) {
 return muscleCarService.create(muscleCar);
 }
 @RequestMapping(method = GET)
 @ResponseStatus(OK)
 List<Map<String, Object>> get() {
 return muscleCarService.read();
 }
 @RequestMapping(value = "/{id}", method = GET)
 @ResponseStatus(OK)
 MuscleCar get(
 @PathVariable("id") final int id
 ) {
 return muscleCarService.read(id);
 }
 @RequestMapping(value = "/{id}", method = PUT)
 @ResponseStatus(OK)
 MuscleCar update(
 @PathVariable("id") final int id,
 @RequestBody final MuscleCar muscleCar
 ) {
 return muscleCarService.update(id, muscleCar);
 }
 @RequestMapping(value = "/{id}", method = DELETE)
 @ResponseStatus(NO_CONTENT)
 void delete(
 @PathVariable("id") final int id
 ) {
 muscleCarService.delete(id);
 }
}

@ControllerAdvice
final class ControllerExceptionAdvice {
 @ExceptionHandler(IllegalArgumentException.class)
 ResponseEntity<String> acceptIllegalArgumentException(final IllegalArgumentException ex) {
 return status(BAD_REQUEST).body("Illegal argument exception: " + ex.getMessage());
 }
 @ExceptionHandler(HttpMessageNotReadableException.class)
 ResponseEntity<String> acceptHttpMessageNotReadableException(final HttpMessageNotReadableException ex) {
 return status(BAD_REQUEST).body("HTTP message not readable exception: " + ex.getMessage());
 }
 @ExceptionHandler(NoSuchElementException.class)
 ResponseEntity<String> acceptNoSuchElementException(final NoSuchElementException ex) {
 return status(NOT_FOUND).body("No such element exception: " + ex.getMessage());
 }
 @ExceptionHandler(Throwable.class)
 ResponseEntity<String> acceptThrowable(final Throwable ex) {
 return status(INTERNAL_SERVER_ERROR).body(ex.getMessage());
 }
}

Service layer

interface IMuscleCarService {
 MuscleCar create(MuscleCar muscleCar);
 List<Map<String, Object>> read();
 MuscleCar read(int id)
 throws NoSuchElementException;
 MuscleCar update(int id, MuscleCar muscleCar)
 throws NoSuchElementException;
 void delete(int id)
 throws NoSuchElementException;
}

@Service
final class MuscleCarService
 implements IMuscleCarService {
 private final IMuscleCarDao muscleCarDao;
 @Autowired
 MuscleCarService(
 final IMuscleCarDao muscleCarDao
 ) {
 this.muscleCarDao = muscleCarDao;
 }
 @Override
 public MuscleCar create(final MuscleCar muscleCar) {
 return muscleCarDao.create(muscleCar);
 }
 @Override
 public List<Map<String, Object>> read() {
 return muscleCarDao.read();
 }
 @Override
 public MuscleCar read(final int id)
 throws NoSuchElementException {
 return requireMuscleCarFound(id);
 }
 @Override
 public MuscleCar update(final int id, final MuscleCar muscleCar)
 throws NoSuchElementException {
 requireMuscleCarFound(id);
 return muscleCarDao.update(id, muscleCar);
 }
 @Override
 public void delete(final int id)
 throws NoSuchElementException {
 requireMuscleCarFound(id);
 muscleCarDao.delete(id);
 }
 private MuscleCar requireMuscleCarFound(final int id)
 throws NoSuchElementException {
 final MuscleCar muscleCar = muscleCarDao.read(id);
 if ( muscleCar == null ) {
 throw new NoSuchElementException(MuscleCar.class.getName());
 }
 return muscleCar;
 }
}

Data layer

interface IMuscleCarDao {
 MuscleCar create(MuscleCar muscleCar);
 List<Map<String, Object>> read();
 MuscleCar read(int id);
 MuscleCar update(int id, MuscleCar muscleCar);
 void delete(int id);
}

@Repository
class MuscleCarDao
 extends JdbcDaoSupport
 implements IMuscleCarDao {
 @Autowired
 MuscleCarDao(
 final JdbcTemplate jdbcTemplate
 ) {
 setJdbcTemplate(jdbcTemplate);
 }
 @Override
 public MuscleCar create(final MuscleCar muscleCar) {
 getJdbcTemplate().update(
 "INSERT INTO muscle_cars (car_brand, car_model, horsepower, car_engine) VALUES (?, ?, ?, ?)",
 muscleCar.getCarBrand(), muscleCar.getCarModel(), muscleCar.getHorsepower(), muscleCar.getCarEngine()
 );
 return muscleCar;
 }
 @Override
 public List<Map<String, Object>> read() {
 return getJdbcTemplate().queryForList(
 "SELECT * FROM muscle_cars"
 );
 }
 @Override
 public MuscleCar read(final int id) {
 return getJdbcTemplate().queryForObject(
 "SELECT * FROM muscle_cars WHERE id = ?",
 MuscleCarDao::mapRowToMuscleCar,
 id
 );
 }
 @Override
 public MuscleCar update(final int id, final MuscleCar muscleCar) {
 getJdbcTemplate().update(
 "UPDATE muscle_cars SET car_brand = ?, car_model = ?, horsepower = ?, car_engine = ? WHERE id =?",
 muscleCar.getCarBrand(), muscleCar.getCarModel(), muscleCar.getHorsepower(), muscleCar.getCarEngine(), id
 );
 return muscleCar;
 }
 @Override
 public void delete(final int id) {
 getJdbcTemplate().update(
 "DELETE FROM muscle_cars WHERE id = ?",
 id
 );
 }
 private static MuscleCar mapRowToMuscleCar(final ResultSet resultSet, @SuppressWarnings("unused") final int i)
 throws SQLException {
 return new MuscleCar(
 resultSet.getString("car_brand"),
 resultSet.getString("car_model"),
 resultSet.getString("horsepower"),
 resultSet.getString("car_engine")
 );
 }
}

• Simon Forsberg: The convention in Java is to not use any I-prefix for interfaces.