2
\$\begingroup\$

I'm making a web-service for an app with a (partially) crowdsourced movie database.

These two functions allows users to add and remove which genres a movie is liked to, and get the log of whom have done what changes.

I need to make many very similar functions for other data.

    [HttpPost]
    [Authorize(Roles = "User")]
    public IHttpActionResult PostMovieGenre([FromBody]MovieGenreBindingModel edit)
    {
        string[] IDs = edit.genre_ids.Split(',');

        using (var db = new DBEntities())
        {
            var movie = db.movies.Find(edit.movie_id);

            if (movie == null)//Confirm that the movie was found
                return BadRequest("Movie not found");

            foreach (string sid in IDs)
            {
                int g_id = Convert.ToInt32(sid);
                if (edit.action == "add")
                {
                    movie.movies_genres.Add(new movies_genres
                    {
                        movies_id = edit.movie_id,
                        genres_id = g_id
                    });
                }
                else if (edit.action == "remove")
                {
                    var m_g = movie.movies_genres.Where(g => g.genres_id == g_id).First(); //Find the movies_genres object
                    db.movies_genres.Remove(m_g); //Remove it
                }
                else
                {
                    return BadRequest("Invalid action");
                }

                db.movies_genres_changes.Add(new movies_genres_changes //Log the change
                {
                    action = edit.action,
                    genres_id = g_id,
                    movies_id = edit.movie_id,
                    time = DateTime.UtcNow,
                    user_id = UserManager.FindByName(User.Identity.Name).Id
                });
            }
            try
            {
                db.SaveChanges();
            }
            catch (Exception e)
            {
                return BadRequest();
            }
        }
        return Ok();
    }


    [ResponseType(typeof(List<RMovieGenresChange>))]
    [HttpGet]
    [Authorize(Roles = "User")]
    public IHttpActionResult GetMovieGenresChanges(int movie_id)
    {
        List<RMovieGenresChange> retchanges = new List<RMovieGenresChange>();

        using (var db = new DBEntities())
        {
            try
            {
                var dbRevs = db.movies.Find(movie_id).movies_genres_changes;

                if (dbRevs == null)
                    return BadRequest("No revision for an movie entry with id " + movie_id);

                foreach (movies_genres_changes m in dbRevs)
                {
                    retchanges.Insert(0, new RMovieGenresChange() // Put newest change first in list
                    {
                        action = m.action,
                        genre = new RGenre
                        {
                            id = m.genres_id,
                            name = m.genres.name
                        },
                        movie_id = m.movies_id,
                        time = m.time,
                        user = new RUserShort
                        {
                            user_id = m.user_id,
                            user_name = User.Identity.Name
                        }
                    });
                }
            }
            catch (Exception e)
            {
                return BadRequest();
            }
        }

        return Ok(retchanges);
    }
\$\endgroup\$
1
\$\begingroup\$

A general comment: Extract logic to small private methods. Note that your methods are long which makes it hard to understand what they do. Every independent piece of code should be extracted into a private method. Like

private void RemoveMovie(id)

If you use the Add method on a List object it will insert the first item into the first place the second to the second and so on. No need in retchanges.Insert(0, new RMovieGenresChange()

GetMovieGenresChanges is a good method since it does only one thing unlike PostMovieGenre method which handles both the addition and the removal of the geners. This is a violation of the Single Responsibility Principle and hence you should split it to two methods: one for the addition AddGenereToMovie and another for the removal - RemoveGenereFromMovie. This way yo keep your methods short, clean, readable and maintainable.

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