HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Wrapper class for the Rotten Tomatoes API

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theapiwrapperforrottenclasstomatoes

Problem

I'm working on a site that has to do with movies. I am consuming the Rotten Tomatoes API to get my movie information and using EF5 with a code first approach to get that into the database. I'm using MVC4. I am struggling to come up with a design that makes sense and accomplishes what I need to in an efficient manner while still following the Single Responsibility Principle.

A quick summary:

Users can have a collection of movies, so every movie that is returned to the client needs to have information like whether the requesting user has that movie in their collection, the user rating, etc. When a user adds a movie to their collection in the database I also insert that movie (into the database). So I'm only inserting movies that users own, as opposed to every movie returned from Rotten Tomatoes.

I think it makes sense to walk you through the action of a user searching for a movie to best show you my problems/questions...

A user searches for a movie and makes a request to my controller. I've created a wrapper class for the Rotten Tomatoes API, so the controller instantiates my RottenTomatoes object then calls the FindMovies() method.

Controller

[HttpGet]
public JsonResult GetMovieSearchResults(string name) {
    if(!string.IsNullOrEmpty(name)) {
        var tomatoes = new RottenTomatoes(ConfigurationManager.AppSettings["RottenTomatoesApiKey"]);
        return Json(tomatoes.FindMovies(name), JsonRequestBehavior.AllowGet);
    }

    return Json(new { success = false }, JsonRequestBehavior.AllowGet);
}


The FindMovies() method calls GetResponse() which makes a call to the API endpoint, deserializes the returned json into MoviesDataContract and then returns that DataContract back to FindMovies().

RottenTomatoes wrapper class

```
public class RottenTomatoes {
private const string MOVIES_SEARCH_URL = "http://api.rottentomatoes.com/api/public/v1.0/movies.json?apikey={0}&q={1}&page_limit={2}";
private Guid _userId;
private MovieCont

Solution

This looks really silly to me:

try {
        using (var response = (HttpWebResponse)request.GetResponse()) {
            var serializer = new DataContractJsonSerializer(typeof(MoviesDataContract));
            return (MoviesDataContract)serializer.ReadObject(response.GetResponseStream());
        }
    } catch (Exception e) {
        throw new Exception(e.Message);
    }


You create an exception that throws an exception. It just doesn't make sense to me. If you want the exception to bubble up, then you should just get rid of the try catch altogether.

It would be different if you returned a "bad response" so that the calling object knew that it was bad for some reason, so that it could tell the user that something went wrong.

You already have a Using Statement in there so that if there is an issue the response is still disposed, so you should just let the exception bubble up naturally.

using (var response = (HttpWebResponse)request.GetResponse()) {
    var serializer = new DataContractJsonSerializer(typeof(MoviesDataContract));
    return (MoviesDataContract)serializer.ReadObject(response.GetResponseStream());
}

Code Snippets

try {
        using (var response = (HttpWebResponse)request.GetResponse()) {
            var serializer = new DataContractJsonSerializer(typeof(MoviesDataContract));
            return (MoviesDataContract)serializer.ReadObject(response.GetResponseStream());
        }
    } catch (Exception e) {
        throw new Exception(e.Message);
    }
using (var response = (HttpWebResponse)request.GetResponse()) {
    var serializer = new DataContractJsonSerializer(typeof(MoviesDataContract));
    return (MoviesDataContract)serializer.ReadObject(response.GetResponseStream());
}

Context

StackExchange Code Review Q#19255, answer score: 6

Revisions (0)

No revisions yet.