1
\$\begingroup\$

I am still a beginner and I would love to hear some feedback on a small wrapper I wrote for the TMDb API (https://developers.themoviedb.org/3/getting-started/introduction). It is used for queries on a movie database. I tried to make it as simple as possible so all that is needed to get relevant information is the title of the movie or tv-show. It is quite basic at the moment since it is not yet finished.

There is only one constructor and it expects the movie title as a String object. This is what a search looks like. I wasn't quite sure what to return if the movie does not exist so it returns empty fields in that case.

MovieDataRetriever someMovie = new MovieDataRetriever("movie title here")
someMovie.getTitle() //returns the title of the movie
someMovie.getReleaseDate() //returns the release date of the movie

I also wrote a static factory method that returns an array of movie objects based on the user query. This is what it looks like in practice

MovieDataRetriever[] movies = getMoviesByQuery("query string here")

The data is obtained as a JSON field from the TMDb servers using HTTP requests. Since movie requests return different fields than tv-show requests, I wrote two seperate classes. I also wrote another abstract superclass where I implemented the behavior that is shared by the other classes (Movieretriever and Tvshowretriever).

This is the abstract superclass MediaDataRetriever

public abstract class MediaDataRetriever
{
    private static final String API_KEY = "<<APIKEY>>";

    /*
     * guarantees to return a valid JsonObject containing data about the specified movie/tv-show
     * returns null if the movie/tv-show does not exist on the TMdb servers or an error occurs while
     * retrieving the JsonObject
     *
     * @param mediaType Media type as a string ("movie" or "tv-show" )
     * @param mediaTitle Title of the movie/tv-show as a string
     * @returns mediaData a valid JsonObject containing all relevant information about the specified movie/tv-show
     */
    protected final JsonObject validateMediaData(final String mediaType, final String mediaTitle)
    {
        try {
            JsonObject mediaData = getMediaDataAsJson(mediaType, mediaTitle);
            return mediaData;
        } catch (IOException e) {
            return null;
        }
    }

    /*
     * private helper method that returns a JSON file that contains complete information about a movie/tv-show
     * as a JsonObject
     *
     * @params mediaType    Media type as a string ("movie" or "tv-show")
     * @param mediaTitle    Title of the movie/tv-show as a string
     * @returns rootObject  a JsonObject containing all relevant information about the searched for movie/tv-show
     * @throws IOException  if an error occurs while decoding the URL's
     */
    private final JsonObject getMediaDataAsJson(final String mediaType, final String mediaTitle) throws IOException
    {
        if(!mediaType.equals("movie") && !mediaType.equals("tv"))
        {
            throw new IllegalArgumentException("Possible values for mediaType are \"movie\" and \"tv\" only");
        }

        JsonParser mediaParser;
        JsonElement rootElement;
        JsonObject rootObject;

        /* the data is obtained with a simple HTTP request.
         * To get detailed movie/tv show information,
         * it is neccessary to do a standard search with limited return values first
         * to obtain the ID of the movie/tv show. The ID is needed to start a detailed search
         * for additional information like the runtime, vote count, vote score, budget ...
         */
        String standardQuery = "https://api.themoviedb.org/3/search/" + mediaType + "?api_key=" + API_KEY + "&query=" + mediaTitle;
        URL standardSearchUrl = new URL(standardQuery);
        HttpURLConnection requestStandardData = (HttpURLConnection) standardSearchUrl.openConnection();
        requestStandardData.connect();

        try(InputStreamReader standardQueryStream = new InputStreamReader((InputStream) requestStandardData.getContent()))
            {
                mediaParser = new JsonParser(); //create a new JsonParser instance
                rootElement = mediaParser.parse(standardQueryStream); //convert the input stream to a json element
                rootObject = rootElement.getAsJsonObject(); //get the element as a json object
            }

        /* The http request returns several fields, the field that contains the movie information is called
         * "results". This field will have a size of zero if the searched for movie does not exist
         */
        if(rootObject.get("results").getAsJsonArray().size() == 0) return null;

        //the movie data is contained in the first sub-field of the results field
        //in this case, we only obtain the movie ID to start a detailed query
        String mediaId = rootObject
                    .get("results")
                    .getAsJsonArray()
                    .get(0)
                    .getAsJsonObject()
                    .get("id")
                    .getAsString();

        //URL format for the detailed query
        String detailedQuery = "https://api.themoviedb.org/3/" + mediaType + "/" + mediaId + "?api_key=" + API_KEY;

        URL detailedQueryUrl = new URL(detailedQuery);
        HttpURLConnection requestDetailed = (HttpURLConnection) detailedQueryUrl.openConnection();
        requestDetailed.connect();

        try(InputStreamReader detailedQueryStream = new InputStreamReader((InputStream) requestDetailed.getContent()))
        {
            rootElement = mediaParser.parse(detailedQueryStream);  //convert the input stream to a json element
            rootObject = rootElement.getAsJsonObject(); //get the json element as a json object
        }

        return rootObject;
    }

    /*
     * returns a gson JsonObject that contains several JSON fields with information required to start a detailed search
     * for a collection of movies/tv-shows based on the user submitted query
     *
     * @param mediaType string representing the media type (movie or tv-show)
     * @param query user submitted query string
     * @returns rootObject a gson JsonObject containing several JSON fields with name and ID fields
     * @throws IOException if an error occurs while decoding the URL
     */
    protected static JsonObject getMediaBasedOnQueryAsJson(final String mediaType, final String query) throws IOException
    {
        if(!mediaType.equals("movie") && !mediaType.equals("tv"))
        {
            throw new IllegalArgumentException("Possible values for mediaType are \"movie\" and \"tv\" only");
        }

        String queryString = "https://api.themoviedb.org/3/search/" + mediaType + "?api_key=" + API_KEY + "&query=" + query.replace(" ", "+");
        URL searchUrl = new URL(queryString);
        HttpURLConnection searchRequest = (HttpURLConnection) searchUrl.openConnection();

        JsonParser mediaParser;
        JsonElement rootElement;
        JsonObject rootObject;

        try(InputStreamReader standardQueryStream = new InputStreamReader((InputStream) searchRequest.getContent()))
            {
                mediaParser = new JsonParser(); //create a new JsonParser instance
                rootElement = mediaParser.parse(standardQueryStream); //convert the input stream to a json element
                rootObject = rootElement.getAsJsonObject(); //get the element as a json object
            }

        return rootObject;
    }

    /*
     * returns the poster of the movie
     *
     * @param posterPath filepath of the poster as described in the TMdb API
     * @returns poster BufferedImage object representing the poster of the movie/tv-show
     * @throws IOException if an error occurs while decoding the URL
     */
    protected BufferedImage getPoster(final String posterPath) throws IOException
    {
        //The poster_path field contains the file path as a String that looks like this: /xxxxxxxxxx.png
        if(posterPath == null) return null;

        //The file path of the poster has to be appended to the base url
        String posterUrl = "https://image.tmdb.org/t/p/w185/" + posterPath;
        URL posterQuery = new URL(posterUrl);
        BufferedImage poster = ImageIO.read(posterQuery);

        return poster;
    }
}

The HTTP requests for movie and tv-shows are similar so I implemented the method that requests the JSON data inside the abstract superclass. The returned JSON data has different fields for movie and tv-shows so the actual parsing is done inside the specific classes (TvDataRetriever, MovieDataRetriever). This is the MovieDataRetriever class described above. I omitted the getter methods since they are very basic. I also omitted the TvDataRetriever class that is used to get information on TV-shows since the actual implementations of movie and tv-show classes are quite similar-

/* Retrieves information about the specified movie as JSON fields using the TMdb API (Version 3)
 * and converts the JSON data into Java objects for easy access
 * Returned values correspond to movie data from the US
 * To get data for other regions, check the TMdb API
 */
public class MovieDataRetriever extends MediaDataRetriever
{
    private final String MOVIE_TITLE_FOR_QUERY;
    private final String MOVIE_TITLE;
    private final String MOVIE_STATUS;
    private final long BUDGET;
    private final long REVENUE;
    private final String[] GENRES;
    private final String RELEASE_DATE;
    private final int RUNTIME;
    private final double VOTE_AVERAGE;
    private final long VOTE_COUNT;
    private final String POSTER_PATH;
    private boolean validityFlag = true;

    public MovieDataRetriever(String movieTitle)
    {
        if(movieTitle == null)
        {
            throw new IllegalArgumentException("Movie title can't be null");
        }
        this.MOVIE_TITLE_FOR_QUERY = movieTitle.replace(" ", "+");
        JsonObject movieData = validateMediaData("movie", MOVIE_TITLE_FOR_QUERY);
        if(movieData == null)
        {
            this.MOVIE_TITLE = "No data";
            this.MOVIE_STATUS = "No data";
            this.BUDGET = 0;
            this.GENRES = new String[0];
            this.RELEASE_DATE = "No data";
            this.REVENUE = 0;
            this.RUNTIME = 0;
            this.VOTE_AVERAGE = 0;
            this.VOTE_COUNT = 0;
            this.POSTER_PATH = "No data";
            validityFlag = false;
        }
        else
        {
            this.MOVIE_STATUS = movieData.get("status").getAsString();
            this.BUDGET = movieData.get("budget").getAsLong();
            this.GENRES = setGenres(movieData);
            this.MOVIE_TITLE = movieData.get("title").getAsString();
            if(movieData.get("release_date") == JsonNull.INSTANCE)
                this.RELEASE_DATE = "Unavailable";
            else
                this.RELEASE_DATE = movieData.get("release_date").getAsString();
            this.REVENUE = movieData.get("revenue").getAsLong();
            if(movieData.get("runtime") == JsonNull.INSTANCE)
                this.RUNTIME = -1;
            else
                this.RUNTIME = movieData.get("runtime").getAsInt();
            this.VOTE_AVERAGE = movieData.get("vote_average").getAsDouble();
            this.VOTE_COUNT = movieData.get("vote_count").getAsLong();
            if(movieData.get("poster_path") == JsonNull.INSTANCE)
                this.POSTER_PATH = null;
            else
                this.POSTER_PATH = movieData.get("poster_path").getAsString();
        }
    }

    @SuppressWarnings("unused")
    private MovieDataRetriever()
    {
        this.MOVIE_TITLE = null;
        this.MOVIE_TITLE_FOR_QUERY = null;
        this.MOVIE_STATUS = null;
        this.BUDGET = 0;
        this.GENRES = null;
        this.RELEASE_DATE = null;
        this.REVENUE = 0;
        this.RUNTIME = 0;
        this.VOTE_AVERAGE = 0;
        this.VOTE_COUNT = 0;
        this.POSTER_PATH = null;
    }

    /* static factory method that returns an array of movie objects
     * based on the query string
     *
     * @params keyword the query string
     * @returns an array of MovieDataRetriever objects (returns a maximum of 20 movies)
     */
    public static MovieDataRetriever[] getMoviesByQuery(String query) throws IOException
    {
        JsonObject rootObject = getMediaBasedOnQueryAsJson("movie", query);

        int searchResultSize = rootObject.get("results")
                                    .getAsJsonArray()
                                    .size();

        if(searchResultSize == 0) return new MovieDataRetriever[0];

        MovieDataRetriever[] searchResults = new MovieDataRetriever[searchResultSize];

        for(int i = 0; i < searchResultSize; i++)
        {
            String movieTitle = rootObject
                                    .get("results")
                                    .getAsJsonArray()
                                    .get(i)
                                    .getAsJsonObject()
                                    .get("title")
                                    .getAsString();
            searchResults[i] = new MovieDataRetriever(movieTitle);
        }

        return searchResults;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Hi Karl, welcome on codreview. I did not (yet) reviewed the whole code. But there is one thing that disrupts me. Usually, we do not want to execute something without telling it explicitly, and based on the name of your classes ("..Retriever") I would expect one method to execute the query: Media result = new MovieRetriever("title").execute(). \$\endgroup\$ – gervais.b Feb 11 '20 at 9:51
1
\$\begingroup\$

As said in my comment, we do not want to execute something without telling it explicitly. Most of the time such programs are built with one class that execute the request and another to model the result.

But let's start by reviewing your code.

The main issue that I see is that you cannot test it without mocking the underlying API. One solution is to introduce an extra, low level, layer that manage the communication and that you can easily mock.

interface MovieDatabase {
    Map<String, Object> search(String mediaType) throws IOException;

    Map<String, Object> get(String mediaType, String mediaId) throws IOException;

    BufferedImage getPoster(String path) throws IOException;
}

I have replaced your JsonObject by a Map<String, Object> so that I can really try to refactor your code without having to manage all the dependencies. But this can also be a pattern if you want to abstract a bit more on the underlying api (It can be an Xml api, or a SQL database)

While we are on that class, you can also replace the String mediaType by one enumeration, so that it will be impossible to ask something else than a movie or a tv show.

When implementing that interface for "The movie db" you will discover that its own responsibility is to execute a query and parse the result. So internally, the public methods will just create a valid url and send it to a private method.

Please not also that URL encoding is more complex than query.replace(" ", "+"), there is one URLEncoder utility class to deal with that.

public Map<String, Object> search(MediaType mediaType, String query) throws IOException {
    return get("search/" + mediaTypeAsString(mediaType) + "&query=" + URLEncoder.encode(query, "UTF-8"));
}

// ...

private Map<String, Object> get(String path) throws IOException {
    URL url = new URL(url+path+"?api_key=" + apiKey);
    HttpURLConnection conn = (HttpURLConnection) url.openConnection();

    try (InputStreamReader standardQueryStream = new InputStreamReader((InputStream) conn.getContent())) {
        return parse(standardQueryStream);
    }
}

private Map<String, Object> parse(InputStreamReader input) {
    // ...
}

At this time you have a low level service to query the api. It can easily be mocked or replaced by another implementation. Let's use it in your MediaDataRetriever, basically you have to add one constructor that expect one MovieDatabase. You can then replace all of the low level calls by a call to the corresponding method on the database. By doing that you will notice that there was a duplication of code in getMediaBasedOnQueryAsJson and validateMediaData, there where both executing a search request.

We can then start to review the MovieDataRetriever. This one use the wrong naming convention for the fields, _UPPER_SNAKE_CASE_ is used for constants and lowerCamelCase must be used for class members. There is also some unused fields.

But since that one inherit from the MediaDataRetriever it wont compile because it doesnt use the super constructor. And this is where the discussion start.

As already said, most of the time, we do not want to execute something without asking for. Usually you ask provide a service that will produce a Media who can be a movie or a tv show. There are some advantages of doing that.

Media media = new MediaService(..).search("..");

In some cases it is interesting to receive a common type. It is also useful in case there is no result because you can either throw one exception or use the "null object pattern".

But, please, note that while the polymorphism is a powerful pattern it may also introduce complexity. If you always know that you want a Movie or a TvShow, receiving one abstract Media can be boring and will introduce many code smells and useless type casts. On that subject you may consider the usage of composition instead of inheritance if your system (business side) never consider that there is a common ancestor to Movie and TvShow.

\$\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.