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

WebApi action that updates entity using stored procedure

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

Problem

I have ASP.NET WebApi project. Some of controller has method SetAsMain(int id) that get information about people then use stored procedure update people. How can I improve it? Should I do check people.IsMain == true in stored procedure?

public HttpResponseMessage SetAsMain(int id)
        {
            People people = repository.GetById(id);

            if (people == null)
            {
                return ErrorMsg(HttpStatusCode.NotFound, string.Format("No people with ID = {1}", id));
            }

            if (people.IsMain == true)
            {
                return ErrorMsg(HttpStatusCode.OK, string.Format("{0} is already set as main", people.Houses.ToString())); 
            }

            try
            {
                var parameters = new Dictionary();
                parameters.Add("PeopleID", id.ToString());                

                repository.ExecProcedure("usp_PeopleSetMain", parameters);
                return Request.CreateResponse(HttpStatusCode.OK, repository.GetById(id)); // some problem here
            }
            catch (Exception ex)
            {
                return ErrorMsg(HttpStatusCode.InternalServerError, ex.Message);
            }
        }

Solution


  • You don't have a people, you have a person.



  • Saying if (person.IsMain == true) is exactly equivalent to if (person.IsMain)



Updated code below.

public HttpResponseMessage SetAsMain(int id)
{
    People person = repository.GetById(id);

    if (people == null)
    {
        return ErrorMsg(HttpStatusCode.NotFound, string.Format("No people with ID = {1}", id));
    }

    if (person.IsMain)
    {
        return ErrorMsg(HttpStatusCode.OK, string.Format("{0} is already set as main", person.Houses.ToString())); 
    }

    try
    {
        var parameters = new Dictionary();
        parameters.Add("PeopleID", id.ToString());                

        repository.ExecProcedure("usp_PeopleSetMain", parameters);
        return Request.CreateResponse(HttpStatusCode.OK, repository.GetById(id)); 
    }
    catch (Exception ex)
    {
        return ErrorMsg(HttpStatusCode.InternalServerError, ex.Message);
    }
}


I would also recommend changing the name of the repository method that returns a person from GetById to GetPersonById. As it is currently, the method name gives us no clue what's going on. I had to figure it out from context.

Code Snippets

public HttpResponseMessage SetAsMain(int id)
{
    People person = repository.GetById(id);

    if (people == null)
    {
        return ErrorMsg(HttpStatusCode.NotFound, string.Format("No people with ID = {1}", id));
    }

    if (person.IsMain)
    {
        return ErrorMsg(HttpStatusCode.OK, string.Format("{0} is already set as main", person.Houses.ToString())); 
    }

    try
    {
        var parameters = new Dictionary<string, string>();
        parameters.Add("PeopleID", id.ToString());                

        repository.ExecProcedure("usp_PeopleSetMain", parameters);
        return Request.CreateResponse(HttpStatusCode.OK, repository.GetById(id)); 
    }
    catch (Exception ex)
    {
        return ErrorMsg(HttpStatusCode.InternalServerError, ex.Message);
    }
}

Context

StackExchange Code Review Q#104676, answer score: 3

Revisions (0)

No revisions yet.