3
\$\begingroup\$

For the first time ever, I'm integrating an MVC website to an ASP.NET Web API 2 web service (both of which are coded by myself).

I'd appreciate it if you can just look this over for me and let me know if I'm on the right track here as far as the integration goes.

I'm working based on this tutorial but I had a little trouble along the way.

What I need to know is:

  • Am I missing anything in either the service controller or the client controller that I should have - any oversight?
  • Is my MVC controller totally thread safe (i.e. is anything going to be blocked with the asyncronysity?
  • Is there a better or more efficient way to do anything that I'm doing here?

This is just a simple request/respond scenario where the web service acts as the middle-man between the application and the database so it's really nothing special.

Here's the code for the web service controller:

public class UserController : ApiController
{
    OrtundServiceEntities db = new OrtundServiceEntities();

    public HttpResponseMessage Get()
    {
        List<User> users = db.Users.ToList();

        HttpResponseMessage response = Request.CreateResponse(HttpStatusCode.OK, users);

        return response;
    }

    public HttpResponseMessage Get(int Id)
    {
        User u = db.Users.Single(x => x.Id == Id);

        HttpResponseMessage response;
        if (u != null)
            response = Request.CreateResponse(HttpStatusCode.OK, u);
        else
            response = Request.CreateResponse(HttpStatusCode.NotFound);

        return response;
    }

    [Route("~/api/user/login")]
    public HttpResponseMessage Post([FromBody]User Source)
    {
        User requestedUser = db.Users.Single(x => x.EmailAddress == Source.EmailAddress);

        HttpResponseMessage response;

        if (requestedUser != null && Hashing.ValidatePassword(Source.Password, requestedUser.Password))
            response = Request.CreateResponse(HttpStatusCode.OK, requestedUser);
        else
            response = Request.CreateResponse(HttpStatusCode.NotFound);

        return response;
    }

    public HttpResponseMessage Post([FromBody]User Source)
    {
        try
        {
            User u = new User();
            u.CreatedBy = Source.CreatedBy;
            u.CreatedOn = Source.CreatedOn;
            u.EmailAddress = Source.EmailAddress;
            u.FirstName = Source.FirstName;
            u.LastName = Source.LastName;
            u.ModifiedBy = Source.ModifiedBy;
            u.ModifiedOn = Source.ModifiedOn;
            u.Password = Source.Password;
            u.RoleId = Source.RoleId;
            u.Role = db.Roles.Single(x => x.Id == u.RoleId);

            db.Users.Add(u);
            db.SaveChanges();

            HttpResponseMessage response = Request.CreateResponse(HttpStatusCode.OK);

            return response;
        }
        catch (Exception ex)
        {
            return ThrowError(ex, "POST USER");
        }
    }

    public HttpResponseMessage Put(int id, [FromBody]User Source)
    {
        try
        {
            User existing = db.Users.Single(x => x.Id == Source.Id);

            if (existing != null)
            {
                existing.CreatedBy = Source.CreatedBy;
                existing.CreatedOn = Source.CreatedOn;
                existing.EmailAddress = Source.EmailAddress;
                existing.FirstName = Source.FirstName;
                existing.LastName = Source.LastName;
                existing.ModifiedBy = Source.ModifiedBy;
                existing.ModifiedOn = Source.ModifiedOn;
                existing.Password = Source.Password;
                existing.RoleId = Source.RoleId;
                existing.Role = db.Roles.Single(x => x.Id == existing.RoleId);

                db.Entry(existing).State = System.Data.Entity.EntityState.Modified;
                db.SaveChanges();

                HttpResponseMessage response = Request.CreateResponse(HttpStatusCode.OK);
                response.Content = new StringContent("Success", Encoding.Unicode);

                return response;
            }
            else
            {
                return ThrowError(new ArgumentNullException("No user record found"), "UPDATE USER");
            }
        }
        catch (Exception ex)
        {
            return ThrowError(ex, "UPDATE USER");
        }
    }

    public HttpResponseMessage Delete(int id)
    {
        User user = db.Users.Find(id);

        HttpResponseMessage response;
        if (user == null)
        {
            response = Request.CreateResponse(HttpStatusCode.NotFound);
        }
        else
        {
            db.Users.Remove(user);
            db.SaveChanges();
            response = Request.CreateResponse(HttpStatusCode.OK);
        }

        return response;
    }


    private HttpResponseMessage ThrowError(Exception ex, string Action = "")
    {
        StringBuilder sb = new StringBuilder();
        sb.AppendLine(String.Format("Error actioning request: [ {0} ]", ex.Message));

        HttpResponseMessage response = Request.CreateResponse(HttpStatusCode.BadRequest, "Error");
        response.Content = new StringContent(ex.ToString(), Encoding.Unicode);

        return response;
    }
}

Here's the controller on the client (MVC) website that uses the above:

public class UserController : Controller
{
    // GET: User
    public async Task<ActionResult> Index()
    {
        List<User> users = new List<User>();
        using (var client = new HttpClient())
        {
            client.BaseAddress = new Uri("http://www.ortund.com/");
            client.DefaultRequestHeaders.Accept.Clear();
            client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

            HttpResponseMessage response = await client.GetAsync("api/user/");

            if (response.IsSuccessStatusCode)
            {
                users = await response.Content.ReadAsAsync<List<User>>();
            }
        }

        return View(users);
    }

    [HttpPost]
    public async Task<ActionResult> Create(User source)
    {
        if (ModelState.IsValid)
        {
            try
            {
                using (var client = new HttpClient())
                {
                    client.BaseAddress = new Uri("http://www.ortund.com/");
                    client.DefaultRequestHeaders.Accept.Clear();
                    client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

                    HttpResponseMessage response = await client.PostAsJsonAsync("api/user", source);

                    if (response.IsSuccessStatusCode)
                    {
                        return Json(new
                            {
                                Message = "Success",
                                IsOK = bool.TrueString
                            });
                    }
                }
                return View();
            }
            catch(Exception ex)
            {
                ThrowError(ex, "CREATE");
            }
        }
        return View();
    }

    [HttpPost]
    public async Task<ActionResult> Edit(User source)
    {
        if (ModelState.IsValid)
        {
            try
            {
                using (var client = new HttpClient())
                {
                    client.BaseAddress = new Uri("http://www.ortund.com/");
                    client.DefaultRequestHeaders.Accept.Clear();
                    client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

                    HttpResponseMessage response = await client.GetAsync(String.Format("api/user/{0}", source.Id));

                    if (response.IsSuccessStatusCode)
                    {
                        Uri ppUrl = response.Headers.Location;

                        // HTTP PUT
                        User user = await response.Content.ReadAsAsync<User>();

                        user.EmailAddress = source.EmailAddress;
                        user.FirstName = source.FirstName;
                        user.LastName = source.LastName;
                        user.ModifiedBy = String.Format("{0} {1}", Request.Cookies["ortundAdmin"]["fname"], Request.Cookies["ortundAdmin"]["lname"]);
                        user.ModifiedOn = DateTime.Now;
                        user.Password = source.Password;

                        response = await client.PutAsJsonAsync(ppUrl.ToString(), user);

                        return Json(new
                        {
                            Message = "Success",
                            IsOK = bool.TrueString
                        });
                    }
                }
                return View(source);
            }
            catch (Exception ex)
            {
                ThrowError(ex, "EDIT");
            }
        }
        return View(source);
    }

    private async Task<ActionResult> Delete(int id)
    {
        string url = String.Format("api/user/{0}", id);

        using (HttpClient client = new HttpClient())
        {
            client.BaseAddress = new Uri("http://www.ortund.com/");
            client.DefaultRequestHeaders.Accept.Clear();
            client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

            HttpResponseMessage response = await client.DeleteAsync(url);

            return RedirectToAction("Index");
        }
    }

    private ActionResult ThrowError(Exception ex, string Action)
    {
        StringBuilder sb = new StringBuilder();
        sb.AppendLine(String.Format("Error actioning request: [ {0} ]", ex.Message));
        return Json(new
        {
            Title = String.Format("{0} USER", Action),
            Message = HttpUtility.HtmlEncode(sb.ToString()),
            Error = ex.ToString()
        });
    }
}
\$\endgroup\$
3
\$\begingroup\$

Avoid duplicated magic strings, like "http://www.ortund.com/". Assign to a constant instead.


This snippet appears repeated multiple times:

client.BaseAddress = new Uri("http://www.ortund.com/");
client.DefaultRequestHeaders.Accept.Clear();
client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

It would be better to move common logic to a helper method.


It's recommended to catch the most specific type of exception, instead of catch (Exception ex). Otherwise you risk masking a truly unexpected failure.


The name of the ThrowError method suggests some sort of helper that throws an exception. But that's not the case: it returns an HTTP error response.


It's not a good idea to name classes after your own name. You should name classes after their functionality, typically the abstract data type they represent.


Instead of storing the value to return in a local variable, like this:

HttpResponseMessage response;
if (u != null)
    response = Request.CreateResponse(HttpStatusCode.OK, u);
else
    response = Request.CreateResponse(HttpStatusCode.NotFound);

return response;

It's somewhat more readable and safer to return immediately, for example:

if (u != null)
{
    return Request.CreateResponse(HttpStatusCode.OK, u);
}
return Request.CreateResponse(HttpStatusCode.NotFound);
\$\endgroup\$
  • \$\begingroup\$ Thanks for that :) very informative. I'm not so sure about how to use helper methods effectively. I know it's bad code repetition, but I can't seem to come up with a way to set that stuff ONCE only and use it everywhere I need to... Unless I just put it in a method that sets it all for each HttpClient and call that... \$\endgroup\$ – Ortund Jul 2 '15 at 21:18
  • \$\begingroup\$ As for the exception handling, it would cause a lot of bloat to have a catch for EVERY exception that could happen and I don't know what exceptions are most likely to happen so I go as broad as possible \$\endgroup\$ – Ortund Jul 3 '15 at 11:06
1
\$\begingroup\$

One small quick point to consider. You appear to be using.

db.<dbset>.Single(func<>)

in most of your methods and then comparing the result to check whether it's null or not.

By definition single expects there to be exactly one element that matches your query. If there is not one element (0), or > 1 an exception will be thrown.

Returns the only element of a sequence, and throws an exception if there is not exactly one element in the sequence.

So in your case if you are expecting NULL back as a possible value i.e. the value might not exist, then you should be looking at using SingleOrDefault() instead.

i.e.

User existing = db.Users.SingleOrDefault(x => x.Id == Source.Id);

if (existing != null)
{
\$\endgroup\$
  • \$\begingroup\$ I'm expecting exactly 1 result or none (for example if a username/password supplied is incorrect). As I'm selecting on the ID of each record which by the time this code is reached, should actually exist, I don't expect any errors despite using .Single() \$\endgroup\$ – Ortund Jul 2 '15 at 20:03
  • \$\begingroup\$ If your expecting possibly none, then my answer still stands \$\endgroup\$ – dreza Jul 2 '15 at 20:15

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.