0
\$\begingroup\$

I want to implement a crud movie library system and also some filtering capabilities.

Since it is so big, I have added the link to the code.

https://github.com/neslihanbozer/Movie-Library.

I am aware it would be extendable in terms of testing. But what else could you suggest?

package com.movie.lib.service.impl;

import com.movie.lib.dto.MovieDto;
import com.movie.lib.entity.Movie;
import com.movie.lib.exception.MovieException;
import com.movie.lib.repository.MovieLibraryRepository;
import com.movie.lib.service.MovieLibraryCommandService;
import org.springframework.stereotype.Service;

import javax.transaction.Transactional;
import javax.validation.Valid;
import java.util.Optional;

@Service
@Transactional
public class MovieLibraryCommandServiceImpl implements MovieLibraryCommandService {

    private final MovieLibraryRepository movieLibraryRepository;

    public MovieLibraryCommandServiceImpl(MovieLibraryRepository movieLibraryRepository) {
        this.movieLibraryRepository = movieLibraryRepository;
    }

    @Override
    public String removeMovie(@Valid Long movieId) {

        try {

            Optional<Movie> movie = movieLibraryRepository.findById(movieId);

            if (movie == null || movie.get().getSoftDelete().equalsIgnoreCase("1")) {
                throw new MovieException("Movie does not exist or already removed");
            } else {
                // Removes a movie via soft delete
                movie.get().setSoftDelete("1");
                movieLibraryRepository.save(movie.get());
            }

            return "Movie : " + movieId + " - Successfully removed";

        } catch (Exception e) {
            throw new MovieException("Movie does not exist or already removed");
        }

    }

    @Override
    public String updateMovie(MovieDto movieDto) {

        try {

            var movie = movieLibraryRepository.findById(movieDto.getId());

            if (movie == null || movie.get().getSoftDelete().equalsIgnoreCase("1")) {
                throw new MovieException("Movie does not exist");
            }

            Movie updatedMovie = Movie.builder()
                    .id(movie.get().getId())
                    .name(movieDto.getName())
                    .genre(movieDto.getGenre())
                    .actor(movieDto.getActor())
                    .duration(movieDto.getDuration())
                    .releaseYear(movieDto.getReleaseYear())
                    .status("1")
                    .softDelete("0")
                    .build();

            movieLibraryRepository.save(updatedMovie);

            return "Movie : " + movie.get().getId() + " - Successfully updated";

        } catch (Exception e) {
            throw new MovieException("Exception while updating movie - " + e.getMessage());

        }
    }

    @Override
    public String addMovie(MovieDto movieDto) {

        try {
            Movie movie = Movie.builder()
                    .id(movieDto.getId())
                    .name(movieDto.getName())
                    .genre(movieDto.getGenre())
                    .actor(movieDto.getActor())
                    .status("1")
                    .duration(movieDto.getDuration())
                    .releaseYear(movieDto.getReleaseYear())
                    .softDelete("0")
                    .build();

            movieLibraryRepository.save(movie);

            return "Movie : " + movieDto.getId() + " - Successfully saved";

        } catch (Exception e) {
            throw new MovieException("Exception while saving movie - " + e.getMessage());
        }

    }

}
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review. Please note that by our rules, external links can only be treated as supplementary material. If you want a review of more than this class, then that code has to be embedded directly in the question too. \$\endgroup\$ Aug 12, 2022 at 17:18

1 Answer 1

1
\$\begingroup\$

Now, I've really only looked at MovieLibraryCommandServiceImpl so far, and haven't looked at much of rest of the repo. Still, I have some comments on what I have seen:

I don't like how status and softDelete are numerical Strings. Which statuses can exist? What do they mean? I feel like using enums for those could communicate this in a clearer fashion

On the subject of Strings, I find it kind of odd that these add/remove/update methods return a log message if, and only if, they succeed. First off, I wouldn't think formatting such messages is part of this class's purpose. Second, the situation where you're most likely to need a log message is when things have gone wrong, and in those situations you don't get a return value from those methods in the first place. Personally, I would change these methods to be void - the caller already knows the ID of the movie they operate on (or they couldn't call these methods), and they know whether the operation succeeded (based on whether the methods return successfully or throw exceptions), so they're perfectly capable of formatting their own messages if they need to

I'm not a big fan of the way Optional is used here. Or, rather, the way Optional seemingly isn't used - half the point of Optional is to gain the ability to have objects do things based on whether or not they have a value (through Optional::ifPresent, Optional::map, Optional::orElseGet, etc). The other half is avoiding null. But this code takes advantage of none of that, only use Optional::get (which fails if the Optional is empty, suggesting that empty Optionals aren't being used). Additionally, there are methods which claim to return an Optional<Movie> but are treated as though they can return null - which is kind of redundant when Optional.empty exists. As much as I like Optional, it's only useful if it's actually used

Finally, the exception handling also raises some concerns to me

  • First, catch (Exception e) is very broad, catching a wide variety of exceptions. This can sometimes be useful, but knowing exactly what you catch when and where is important, so you can respond appropriately. This code catches almost everything indicriminately, and handles it all the same way, which is usually not ideal. If you want, you can still commit to handling every failure state you can imagine, but right now you're also committing to handling every failure state you can't imagine, and that rarely ends well. Additionally, specifying which exception type(s) you're expecting where usually makes the code easier to understand
  • Second, in those catch blocks, you create, and throw, a brand new exception, losing all information about the old one. So when something fails, all you get is a MovieException with little to no detail. That seems like a nightmare when debugging, as you'll have no idea what actually caused the failure. Personally, I'd suggest removing those try/catch blocks entirely, and just letting the original exception bubble up until someone can handle it properly. But if you really don't want that for some reason you should at least set the original exception as that new MovieException's cause, so it can show up in stacktraces and such
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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