4
\$\begingroup\$

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();
    }
}
\$\endgroup\$
4
\$\begingroup\$

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.

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