3
\$\begingroup\$

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<string, string>();
                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);
            }
        }
\$\endgroup\$
  • \$\begingroup\$ I was afraid of this when I saw your previous question: why are you using a stored proc to update this one property? Also, this seems contradictory to me: ErrorMsg(HttpStatusCode.OK -- what is it, an error or not an error? \$\endgroup\$ – BCdotWEB Sep 15 '15 at 9:01
  • \$\begingroup\$ This stored proc not only update one property its do many thing. About ErrorMsg: I have to create a similar method with other name. Thanks \$\endgroup\$ – Roman Marusyk Sep 15 '15 at 9:29
3
\$\begingroup\$
  • 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<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);
    }
}

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks. A second time I want to get not the same instance of people, it should return a updated data after run stored proc. but it does not work. Could you please suggest me? \$\endgroup\$ – Roman Marusyk Sep 15 '15 at 9:52
  • \$\begingroup\$ Ahh. Yes. Good catch. You are getting a new instance back after it's been updated. You can ignore that part of my answer. I was wrong about that. \$\endgroup\$ – RubberDuck Sep 15 '15 at 10:39
  • \$\begingroup\$ Yes, but it return me the same instance as I had before without changes :(. What is I doing wrong? Thx. \$\endgroup\$ – Roman Marusyk Sep 15 '15 at 11:01
  • 1
    \$\begingroup\$ @MegaTron I think it makes sense to do the check where you do it. If it's already set to be "main", there's no sense in making another round trip to the database. \$\endgroup\$ – RubberDuck Sep 15 '15 at 11:39
  • 1
    \$\begingroup\$ Your welcome. Good luck with working out your other issue. One thing to check for is if your sproc is actually updating the record. \$\endgroup\$ – RubberDuck Sep 15 '15 at 11:41

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.