MVC API controller processing POST request

Question

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.

Answer 1

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.

• Kai: thanks for your answer, those two articles were actually quite helpful in my case! I will reorganize my code and reduce unnecessary comments.