Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

It return an object with WebAPI by proving some data. Is this a good approach?

using MWM.Database.Model;
using MWM.Entity.API;
using MWM.Repository;
using MWM.Services;
using System.Net;
using System.Net.Http;
using System.Web.Http;

namespace MWM.Web.Controllers
{
    public class JobController : ApiController
    {
        private readonly IJobRepository _jobRepo;
        private readonly JobFacade _jobFacade;

        public JobController()
        {
            // Init repository
            MwmContext context = new MwmContext();

            // WebAPI is not able to serialize objects with this feature enabled.
            context.Configuration.ProxyCreationEnabled = false;
            _jobRepo = new JobRepository(context);
            _jobFacade = new JobFacade(_jobRepo);
        }

        /// <summary>
        /// Get information related with a job by providing "Custome surname" and "VRN".
        /// </summary>
        /// <param name="surname">Customer surname.</param>
        /// <param name="vrn">Vehicle registration number.</param>
        /// <returns>Returns a collection of fields related to the job.</returns>
        [Authorize(Users = "WEBSERVICE")]
        public HttpResponseMessage Get(string surname, string vrn)
        {
            try
            {
                if (string.IsNullOrEmpty(surname) || string.IsNullOrEmpty(vrn))
                {
                    var message = string.Format("You need to provide both surname and VRN, in order to get some job infomation. (Surname='{0}', VRN='{1}')", surname, vrn);
                    return Request.CreateErrorResponse(HttpStatusCode.NotFound, message);
                }

                APIJob job = _jobFacade.GetJobBySurnameVRN(surname, vrn);
                if (job == null)
                {
                    var message = string.Format("Sorry, but there is no job with surname = {0} and VRN = {1} not finished was found.", surname, vrn);
                    return Request.CreateErrorResponse(HttpStatusCode.NotFound, message);
                }

                return Request.CreateResponse(HttpStatusCode.OK, job);
            }
            catch (System.Exception)
            {
                return Request.CreateErrorResponse(HttpStatusCode.NotFound, "");
            }
        }

    }
}

UPDATE:

This is how the method looks now after the code review:

[HttpGet]
[Authorize(Users = "webservice")]
public IHttpActionResult Get(string surname, string vrn)
{
    try
    {
        if (string.IsNullOrEmpty(surname) || string.IsNullOrEmpty(vrn))
            return BadRequest("Please, provide customer surname and VRN");

        ApiJob job = _jobFacade.GetApiJob(surname, vrn);
        if (job == null) return NotFound();

        return Ok(job);
    }
    catch (System.Exception)
    {
        return Conflict();
    }
}
share|improve this question

1 Answer 1

up vote 4 down vote accepted

I see this pattern from time to time but I don't like it: GetJobBySurnameVRN or GetJobById. That's what overloads and perhaps named arguments are for: all you care about is the GetJob part, everything else is just one specific way of retrieving the data but does not have to actually be a separate method rather than an overload.

I would change it to GetJob and call it like this (and considering your variables are aptly named it can stay like this): GetJob(surname, vrn). If for some reason you don't like this, you can always do GetJob(surname: surname, vrn: vrn) but that doesn't add anything so use it when the variable actually isn't clear (like GetJob(surname: param[0], vrn: param[1])).


Have you considered Web Api 2? It changes this

return Request.CreateErrorResponse(HttpStatusCode.NotFound, "lala");

into

return NotFound("lala");

You'll have these helper methods available for several status codes.


Furthermore, not entirely sure if it's limited to Web Api or Web Api 2 but I prefer to explicitly define the action's attributes: [HttpGet]. Likewise Web Api 2 provides very handy (and very, very configurable) [Route] attributes.

share|improve this answer
    
Thank you Jeroen that looks very good for me. –  tribet Oct 22 '14 at 9:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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