- Request mappings are not that RESTful due to the verbs:
POST /api/cars/add-car=>POST /api/carsGET /api/cars/cars=>GET /api/carsGET /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}
POSTcan return HTTP 201 Created.deleteMuscleCarcauses 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@SuppressWarningsannotation).- There are methods named
remove*. They must be nameddelete*. See the difference herehere. - 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-caronly parameter is annotated with@RequestBodyand the body is required by default, so the service "create" method may have thenullcheck removed.POST /api/cars/add-carshould return a created entity.GET /api/cars/carscannot 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.@ResponseStatusis used to set default response statuses. - Having no results must return an empty collection, never
null. Note howqueryForListworks. - Technically, IDs can be negative.
MuscleCarDaoImpldependencies can be changed: you can omitDataSourceand configure it in a more appropriate place like configuration files.
replaced http://english.stackexchange.com/ with https://english.stackexchange.com/
- Request mappings are not that RESTful due to the verbs:
POST /api/cars/add-car=>POST /api/carsGET /api/cars/cars=>GET /api/carsGET /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}
POSTcan return HTTP 201 Created.deleteMuscleCarcauses 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@SuppressWarningsannotation).- There are methods named
remove*. They must be nameddelete*. 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-caronly parameter is annotated with@RequestBodyand the body is required by default, so the service "create" method may have thenullcheck removed.POST /api/cars/add-carshould return a created entity.GET /api/cars/carscannot 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.@ResponseStatusis used to set default response statuses. - Having no results must return an empty collection, never
null. Note howqueryForListworks. - Technically, IDs can be negative.
MuscleCarDaoImpldependencies can be changed: you can omitDataSourceand configure it in a more appropriate place like configuration files.
- Request mappings are not that RESTful due to the verbs:
POST /api/cars/add-car=>POST /api/carsGET /api/cars/cars=>GET /api/carsGET /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}
POSTcan return HTTP 201 Created.deleteMuscleCarcauses 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@SuppressWarningsannotation).- There are methods named
remove*. They must be nameddelete*. 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-caronly parameter is annotated with@RequestBodyand the body is required by default, so the service "create" method may have thenullcheck removed.POST /api/cars/add-carshould return a created entity.GET /api/cars/carscannot 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.@ResponseStatusis used to set default response statuses. - Having no results must return an empty collection, never
null. Note howqueryForListworks. - Technically, IDs can be negative.
MuscleCarDaoImpldependencies can be changed: you can omitDataSourceand configure it in a more appropriate place like configuration files.
Should be changed:
- Request mappings are not that RESTful due to the verbs:
POST /api/cars/add-car=>POST /api/carsGET /api/cars/cars=>GET /api/carsGET /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}
POSTcan return HTTP 201 Created.deleteMuscleCarcauses 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@SuppressWarningsannotation).- There are methods named
remove*. They must be nameddelete*. 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-caronly parameter is annotated with@RequestBodyand the body is required by default, so the service "create" method may have thenullcheck removed.POST /api/cars/add-carshould return a created entity.GET /api/cars/carscannot 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.@ResponseStatusis used to set default response statuses. - Having no results must return an empty collection, never
null. Note howqueryForListworks. - Technically, IDs can be negative.
MuscleCarDaoImpldependencies can be changed: you can omitDataSourceand configure it in a more appropriate place like configuration files.
Depends on code conventions, style and preferences:
- Java annotations
valuecan 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
finaland create an@AutoWiredconstructor. - I would not use
publicmodifier as much as possible. Spring Framework is smart enough to find not public components. MuscleCarServiceinterface might be introduced in order to have loose coupling.Collection.size() > 0can be replaced with!Collection.isEmpty().- Throwing business exceptions could be more preferable rather than returning special values. Say,
NoSuchElementExceptionmay 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 shorterfinal Object[] args = { foo, bar, baz }.queryForObjectmapper and args can be swapped in favor of varargs methods (variadic parameters).updateis also variadic.- Note that
MuscleCarRowMappercan 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 theRowMapperinterface with a method reference).
My personal preferences:
- I would name interface with starting with
I. Say, I would renameFooandFooImpltoIFooandFoorespectively. In my opinion,*Impllooks worse thanI*. - 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")
);
}
}
lang-java