Czy kontroler ma tylko wywoływać metody?

Czy kontroler ma tylko wywoływać metody i nie zawierać żadnej logiki?
Taki prosty przykład, który sposób jest lepszy.

    //Kod z Kontrolera obecnie
    @PostMapping("/add")
    public ResponseEntity<?> addCar(@RequestBody Car car) {
        carService.save(car);
        if (car.getId != null) {
            return ResponseEntity.ok(car);
        }
        return new ResponseEntity<Object>(HttpStatus.BAD_REQUEST);
    }


    //Czy lepiej jak to będzie w klasie serwisu, a w kontrolerze tylko wyloanie metody
    public ResponseEntity<?> save2(@RequestBody Car car) {
        carRepository.save(car);
        if (car.getId != null) {
            return ResponseEntity.ok(car);
        }
        return new ResponseEntity<Object>(HttpStatus.BAD_REQUEST);
    }

    //Kod kontrolerza
    @PostMapping("/add")
    public ResponseEntity<?> addCar(@RequestBody Car car) {
    	return carService.save2(car);
    }

Drugie pytanie jak przechowywać hasła w bazie danych, wiadomo, że nie powinien być to jawny tekst. Można użyć org.apache.commons.codec i np.
DigestUtils.sha1(password)); zwraca tablicę bajtów, więc w bazie będzie to blob, czy lepiej tablicę bajtów przekonwertować do String64 i w bazie użyć pola też typu varchar?

1. Za tego nulla to powinni drzeć z ciebie pasy ;)
2. Zróbże jedną generyczną metodę która buduje to http entity na podstawie optionala. Coś w stylu

   public static <T> ResponseEntity<T> formResponse(Optional<T> data) {
        return data.map(ResponseEntity::ok)
                .orElse(ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(null));
    }

3. Zwracaj z serwisu Optional i na jego podstawie generuj wynik.
4. Kontroler nie zawiera logiki, ale zwrócenie erroru http to nie logika biznesowa!

I teraz masz

    @PostMapping("/add")
    public ResponseEntity<SomeClass> addCar(@RequestBody Car car) {
        return Utils.formResponse(carService.save(car));
    }

A serwis ma

class CarService{
    public Optional<SomeClass> save(Car car){
        // cośtam
    }
}

Idąc dalej możesz zrobić tam Either zamiast Optional jeśli możliwości ma być więcej.

Dlaczego nulle są złe? A prawidłowe rozwiązanie dla Javy 1.6?

Na temat **nulla **dużo napisano, ale jako podsumowanie najlepiej starczy tekst "twórcy":

A co do logiki w kontrolerach etc. -wszystko zależy troszkę od skali. Im większy wieżowiec tym grubsze ściany i fundamenty muszą być.
A to kosztuje...

Ale jak sobie sam coś robisz małego (szałas, domek letniskowy) to upraszczanie sobie życia nie jest niczym złym. Naprawdę nie musisz pisać "PetStore" w siedmiu warstwach..

Co więcej (moim zdaniem )czasem lepiej zrobić sobie syf i zobaczyć ból na własne oczy, tak żeby widzieć dlaczego.
Jeśli będziesz zawsze słuchał wszelkich rad i good practice bez zrozumienia skąd one pochodzą to zaczniesz uprawiać Cargo Cult Programming. To czasem gorsze od wrzucenia wszystkiego w jeden wielki public static void main

A dlaczego nie zawsze warto wepchać cały kod w kontrolery? Bo jak wepchniesz to znacznie trudniej Ci będzie pokryć testami logikę biznesową twojej aplikacji. A po wydzieleniu zostajesz z prostymi klasami (serwisy, repo ) bez kontekstu HTTP i tym podobnego badziewia. Profit. Łatwiej też takie klasy analizować. I od razu wniosek: nie masz testów to i większości tego zysku nie widać...

Co do haseł to proste pojedyncze hashowanie - nawet z solą to już nie jest dobre rozwiązanie ( od 12 lat....)
Czytaj tu https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet
I ogólnie wszelkie sprawy związane z Security zaczynaj od tej strony (OWASP).

Ale tu znowu wchodzi problem czy budujesz szałas czy wieżowiec. Polecam nie trzymać żadnych haseł i wyniesienie się na autentykację Google+, Facebook, lub Twitter (OAUTH). Odpada dużo odpowiedzialności.

A jak postepowac w przypadku update'u encji. Z kontrolera przychodiz DTO, ktore moze miec w sobie nulle, i teraz chcielibysmy zupdateowac wartosci bazowej encji tylko tam gdzie nulli nie ma. Znalazłem rozwiazanie w ktorych sprawdzanie odbywa się na setterze, ale to rozwiazanie ma wady. Czy też Optionale?

To zrób DTO z Option. To dokładnie dobry przypadek. Jak masz szczęście i jacksona + javaslang i Restowy serwis to chyba nawet dostaniesz w prezencie konwersję JSON-owego nulla na Option z javaslang.
(https://github.com/javaslang/javaslang-jackson)

@filemonczyk ładny offtop :P Ja bym zrobił jeszcze inaczej. Pobrałbym z bazy istniejący obiekt, podmienił parametry z DTO które nie są nullami i zapisał obiekt do bazy.

Shalom napisał(a):

1. Za tego nulla to powinni drzeć z ciebie pasy ;)
2. Zróbże jedną generyczną metodę która buduje to http entity na podstawie optionala. Coś w stylu

   public static <T> ResponseEntity<T> formResponse(Optional<T> data) {
        return data.map(ResponseEntity::ok)
                .orElse(ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(null));
    }

3. Zwracaj z serwisu Optional i na jego podstawie generuj wynik.
4. Kontroler nie zawiera logiki, ale zwrócenie erroru http to nie logika biznesowa!

I teraz masz

    @PostMapping("/add")
    public ResponseEntity<SomeClass> addCar(@RequestBody Car car) {
        return Utils.formResponse(carService.save(car));
    }

A serwis ma

class CarService{
    public Optional<SomeClass> save(Car car){
        // cośtam
    }
}

Idąc dalej możesz zrobić tam Either zamiast Optional jeśli możliwości ma być więcej.

A nie lepiej w serwisie rzucac wyjatek i pozniej go mapowac na odpowiednia odpowiedz?
W sensie cos takiego mam na ymsli:

@Override
   public Tweet readOneTweetFromUser(long userId, long tweetId) {
       Optional<Tweet> tweet = tweetRepository.findByTweetIdAndUserId(tweetId,userId);
       return tweet.orElseThrow(()->  new EntityNotFoundException("Tweet with given id not found"));
   }

@ResponseStatus(HttpStatus.NOT_FOUND)
public class EntityNotFoundException extends RuntimeException {

   public EntityNotFoundException(String message) {
       super(message);
   }
}