4
\$\begingroup\$

I would like to ask more experienced developers to review my code, it's my first app, self-made, I know there is a lot to improve, it would be great if you could just point the main errors.

I'm aware that my implementation is not universal, but I couldnt get past it. My edit option with usage of DTO seems shi*y to me, details can be found in the code.

https://github.com/filemonczyk/crud

Thanks to all in advance

@Controller
public class AnimalsController {

@Autowired
private AnimalDAO animalDAO;

@RequestMapping("")
public String startPage()
{
    return "home";
}


//gives a list of animals of specified type (dog,cat,snake)
@RequestMapping("write{animal}")
public String getAnimalList(Model model, @PathVariable("animal") String animal ) {
    model.addAttribute("animalName", animalDAO.getAnimalList(animal));
    return "list";
}




@RequestMapping("{animal}/{name}/{id}/delete")
public String dropAnimal(@PathVariable("id") int id, HttpServletResponse response) throws IOException{

    animalDAO.delete(id);
    response.sendRedirect("http://localhost:8080/crud");
    return "home";
}


//following 3 methodes are adding new animals to the database, I have used here DTO, had no idea how I can turn it into more flexible solutions
@RequestMapping("addCat")
public String addAnimal(HttpServletRequest request,HttpServletResponse response, @ModelAttribute("catDto") @Valid CatDTO catDto, BindingResult result) throws IOException{
    if(request.getMethod().equalsIgnoreCase("post") && !result.hasErrors()){

        Cat cat = new Cat();
        cat.setName(catDto.getName());
        cat.setAge(catDto.getAge());
        cat.setDateOfBirth(catDto.getBirthDay());
        cat.setBreed(catDto.getBreed());
        cat.setWeight(catDto.getWeight());
        cat.setFurColor(catDto.getFurColor());  
        animalDAO.addAnimal(cat);

        response.sendRedirect("http://localhost:8080/crud");
        return "home";
    }
    return "addCat";
}
@RequestMapping("addDog")
public String addAnimal(HttpServletRequest request,HttpServletResponse response, @ModelAttribute("dogDto") @Valid DogDTO dogDto, BindingResult result) throws IOException{
    if(request.getMethod().equalsIgnoreCase("post") && !result.hasErrors()){

        Dog dog = new Dog();
        dog.setName(dogDto.getName());
        dog.setAge(dogDto.getAge());
        dog.setDateOfBirth(dogDto.getBirthDay());
        dog.setWeight(dogDto.getWeight());
        dog.setTrained(dogDto.isTrained());
        dog.setOwnerName(dogDto.getOwnerName());
        animalDAO.addAnimal(dog);

        response.sendRedirect("http://localhost:8080/crud");
        return "home";
    }

    return "addDog";
}

@RequestMapping("addSnake")
public String addAnimal(HttpServletRequest request,HttpServletResponse response, @ModelAttribute("snakeDto") @Valid SnakeDTO snakeDto, BindingResult result) throws IOException{
    if(request.getMethod().equalsIgnoreCase("post") && !result.hasErrors()){

        Snake snake = new Snake();
        snake.setName(snakeDto.getName());
        snake.setAge(snakeDto.getAge());
        snake.setDateOfBirth(snakeDto.getBirthDay());
        snake.setWeight(snakeDto.getWeight());
        snake.setLength(snakeDto.getLength());
        snake.setVenomous(snakeDto.isVenomous());
        animalDAO.addAnimal(snake);

        response.sendRedirect("http://localhost:8080/crud");
        return "home";
    }

    return "addSnake";
}


// here is the core of my doubts, i wasnt able to configure view-model communication in that way I was not forced 
//to "hard-type" 3 different @ModelAttributes with 3 different object types. 

@RequestMapping("{animal}/{name}/{id}")
public String getAnimal(HttpServletResponse response,Model model, HttpServletRequest request, @PathVariable("animal") String name, @PathVariable("id") int id,
        @ModelAttribute("snakeDto") SnakeDTO snakeDto, @ModelAttribute("catDto") CatDTO catDto ,@ModelAttribute("dogDto") DogDTO dogDto) throws IOException{

    if(request.getMethod().equalsIgnoreCase("post")&&name.equalsIgnoreCase("snake")){
        Snake an = (Snake)animalDAO.getAnimalById(id);
        an.setAge(snakeDto.getAge());
        an.setDateOfBirth(snakeDto.getBirthDay());
        an.setLength(snakeDto.getLength());
        an.setName(snakeDto.getName());
        an.setWeight(snakeDto.getWeight());
        an.setVenomous(snakeDto.isVenomous());
        animalDAO.updateAnimal(an);
        response.sendRedirect("http://localhost:8080/crud");
        return "home";
    }
    if(request.getMethod().equalsIgnoreCase("post")&&name.equalsIgnoreCase("cat")){
        Cat an = (Cat)animalDAO.getAnimalById(id);
        an.setAge(catDto.getAge());
        an.setDateOfBirth(catDto.getBirthDay());
        an.setBreed(catDto.getBreed());
        an.setName(catDto.getName());
        an.setWeight(catDto.getWeight());
        an.setFurColor(catDto.getFurColor());
        animalDAO.updateAnimal(an);
        response.sendRedirect("http://localhost:8080/crud");
        return "home";
    }
    if(request.getMethod().equalsIgnoreCase("post")&&name.equalsIgnoreCase("dog")){
        Dog an = (Dog)animalDAO.getAnimalById(id);
        an.setAge(dogDto.getAge());
        an.setDateOfBirth(dogDto.getBirthDay());
        an.setTrained(dogDto.isTrained());
        an.setName(dogDto.getName());
        an.setWeight(dogDto.getWeight());
        an.setOwnerName(dogDto.getOwnerName());
        animalDAO.updateAnimal(an);
        response.sendRedirect("http://localhost:8080/crud");
        return "home";
    }


    model.addAttribute("animalDetails", animalDAO.getAnimalById(id));
    return "details"+name;
}

}

this is corresponding view:

<%@ page language="java" contentType="text/html; charset=ISO-8859-1"
pageEncoding="ISO-8859-1"%>
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"   "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1">
<title>Insert title here</title>
</head>
<body>
<form method="post" methodAttribute="catDto">
Insert Cat's name 
<input type="text" name="name"/><br>
Insert Cat's age
<input type="text" name="age"/><br>
Insert Cat's date of birth (dd-mm-yyyy)
<input type="text" name="birthDay"><br>
Insert Cat's weight
<input type="text" name="weight"/><br>
Insert Cat's breed?
<input type="text" name="breed"/><br>
Insert Cat's fur color
<input type="text" name="furColor"/> <br>
<input type="submit" value="add"/>




</form>
</body>
</html>
\$\endgroup\$
3
\$\begingroup\$

This is by no means a comprehensive review, just some little things that I would change.

  1. You are often injecting HttpServletRequest and HttpServletResponse as method arguments. This is not necessary and bloats the method signatures. You can just define two instance fields of those two classes and annotate them with @Autowired. Spring creates proxies for those classes and offers the correct instances to your request threads.

  2. Run a code formatter / cleaner on your code before comitting! You should enable "format code on save" in your IDE, if you use one.

  3. You do not need to and probably shouldn't call request.getMethod().equalsIgnoreCase("post") the way you do: You can define the HTTP methods you want to accept in the @RequestMapping annotation via method=RequestMethod.POST. You can define multiple methods with the same path but different methods.

  4. If you decide to put a comment above a method, do it in the proper JavaDoc style.

  5. You should not use absolute redirects like: response.sendRedirect("http://localhost:8080/crud");, use relative paths instead since this would break if your application were to run on another server/port.

  6. Your entities do not use JPA constraints like nullable. Your animals name should probably not be nullable for example!

  7. You might want to implement equals and hashcode for your DTOs and entities, or let your IDE do it for you.

  8. You would benefit from using Spring Boot for applications like this. With Spring Boot you do not need to define applicationContext.xml and web.xml. It's very easy to use and does not require changes to applications on this level.

  9. You would benefit from using spring-data which brings default implementations for the DAO pattern (JPARepository interface and its relatives).

  10. Your controllers might contain too much logic. They should call @Component/@Service bean methods that contain the DB logic. This serves "separation of concerns" and gives you code that can be called from elsewhere.

  11. Your entity method public void setId(int id) should probably not be public. When would you ever want to manipulate the entities id? Let your JPA provider take care of it.

\$\endgroup\$

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.