1
\$\begingroup\$

Do I need to restructure for readability?

 public HttpResponseMessage Post([FromBody]BoatProperties props)
    {
        // model validation
        if(ModelState.IsValid)
        {
            // check if guid is set, else set it; don't set when user is admin
            if(props.Guid == null && !User.IsInRole("Admin"))
            {
                props.Guid = Membership.GetUser().ProviderUserKey as Guid?;
            }

            Boat boat = BoatProvider.GetBoat(props.Guid);

            try
            {
                // can't find existing boat: create new boat
                if (boat == null)
                {
                    boat = new Boat(props);
                    BoatProvider.AddNew(boat);
                }
                // otherwise update existing boat
                else
                {
                    BoatProvider.Update(boat);
                }
            }
            catch(Exception ex)
            {
                // error while updating/adding model
                return Request.CreateErrorResponse(HttpStatusCode.InternalServerError, ex.Message);
            }
            // everything ok!
            return Request.CreateResponse(HttpStatusCode.OK, BoatProvider.GetBoat(props.Guid));
        }
        // validation failed
        return CreateResponse(HttpStatusCode.BadGateway);
    }  

As you can see I'm exiting all those conditions with a return statement. If I restructure my code like this:

if(!ModelState.IsValid)
{
    return CreateResponse(HttpStatusCode.BadGateway);
}
if (props.Guid == null  && !User.IsInRole("Admin"))
{
    props.Guid = guid;
}

I will have less indentations and it may be more readable and all the steps seem more distanced from each other.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I'd certainly try to reduce the indentations, not in the least because in the case of if(ModelState.IsValid) it involves a 30-line block, and the else is just a single line. Also: if you're going to fail, do it fast.

But there are other issues to consider.

  • You expect an Exception to happen when you create or update a Boat? That seems odd.
  • I understand the GetBoat then AddNew vs Update logic, but I'd move that out of the Controller and into a Business layer. Keep your Controllers clean. Right now this is a fairly simply operation and already there are twenty or so lines. Just imagine the mess it'll become once you need to do more complicated operations.
  • I'm also a bit mystified by the logic WRT the update: you retrieve the boat and then you do BoatProvider.Update(boat); -- shouldn't you have done something with that boat first?
  • Most/all of your comments are superfluous. Comments shouldn't tell me what is happening (that's what your code your do), but why.
\$\endgroup\$
1
  • \$\begingroup\$ thanks for your answer, those two articles were actually quite helpful in my case! I will reorganize my code and reduce unnecessary comments. \$\endgroup\$
    – Kai
    Feb 23, 2016 at 7:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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