asp.net mvc related, mainly a refactor question.

Question

can anyone think of a better way to do this?

 [AcceptVerbs(HttpVerbs.Post)]
 public ActionResult SaveAction()
 {
  NameValueDeserializer value = new NameValueDeserializer();

  // selected messages
  MemberMessageSaveAction[] messages = (MemberMessageSaveAction[])value.Deserialize(Request.Form, "value", typeof(MemberMessageSaveAction[]));

  // selected action
  MemberMessageAction action = (MemberMessageAction)Enum.Parse(typeof(MemberMessageAction), Request.Form["action"]);

  // determine action
  if (action != MemberMessageAction.MarkRead &&
   action != MemberMessageAction.MarkUnRead &&
   action != MemberMessageAction.Delete)
  {
   // selected action requires special processing
   IList<MemberMessage> items = new List<MemberMessage>();

   // add selected messages to list
   for (int i = 0; i < messages.Length; i++)
   {
    foreach (int id in messages[i].Selected)
    {
     items.Add(MessageRepository.FetchByID(id));
    }
   }

   // determine action further
   if (action == MemberMessageAction.MoveToFolder)
   {
    // folders
    IList<MemberMessageFolder> folders = FolderRepository.FetchAll(new MemberMessageFolderCriteria
    {
     MemberID = Identity.ID,
     ExcludedFolder = Request.Form["folder"]
    });

    if (folders.Total > 0)
    {
     ViewData["messages"] = items;
     ViewData["folders"] = folders;

     return View("move");
    }

    return Url<MessageController>(c => c.Index("inbox", 1)).Redirect();
   }
   else if (action == MemberMessageAction.ExportXml)
   {
    return new MemberMessageDownload(Identity.ID, items, MemberMessageDownloadType.Xml);
   }
   else if (action == MemberMessageAction.ExportCsv)
   {
    return new MemberMessageDownload(Identity.ID, items, MemberMessageDownloadType.Csv);
   }
   else
   {
    return new MemberMessageDownload(Identity.ID, items, MemberMessageDownloadType.Text);
   }
  }
  else if (action == MemberMessageAction.Delete)
  {
   for (int i = 0; i < messages.Length; i++)
   {
    foreach (int id in messages[i].Selected)
    {
     MemberMessage message = MessageRepository.FetchByID(id);

     if (message.Sender.ID == Identity.ID || message.Receiver.ID == Identity.ID)
     {
      if (message.Sender.ID == Identity.ID)
      {
       message.SenderActive = false;
      }
      else
      {
       message.ReceiverActive = false;
      }

      message.Updated = DateTime.Now;

      MessageRepository.Update(message);

      if (message.SenderActive == false && message.ReceiverActive == false)
      {
       MessageRepository.Delete(message);
      }
     }
    }
   }
  }
  else
  {
   for (int i = 0; i < messages.Length; i++)
   {
    foreach (int id in messages[i].Selected)
    {
     MemberMessage message = MessageRepository.FetchByID(id);

     if (message.Receiver.ID == Identity.ID)
     {
      if (action == MemberMessageAction.MarkRead)
      {
       message.ReceiverRead = true;
      }
      else
      {
       message.ReceiverRead = false;
      }

      message.Updated = DateTime.Now;

      MessageRepository.Update(message);
     }
    }
   }
  }

  return Url<MessageController>(c => c.Index("inbox", 1)).Redirect();
 }

Answer 1

The first step will be making different methods for each action.

Next is to remove the negative logic.

This results in something like this:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult SaveAction() {
  // SNIP   
  if (action == MemberMessageAction.Delete) {
    return DoDeleteAction(...);
  }
  else if (action == MemberMessageAction.MoveToFolder) {
    return DoMoveToFolderAction(...);
  }
  else if (action == MemberMessageAction.ExportXml) {
    return DoExportXmlAction(...);
  }
  else if (action == MemberMessageAction.ExportCsv) {
    return DoExportCsvAction(...);
  }
  else {
    return HandleUnknownAction(...);
  }
}

Answer 2

Turn MemberMessageAction into a class that has a Perform virtual function.

For your Special actions, group the common Perform code:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult SaveAction()
{
    NameValueDeserializer value = new NameValueDeserializer();
    MemberMessageSaveAction[] messages = (MemberMessageSaveAction[])value.Deserialize(Request.Form, "value", typeof(MemberMessageSaveAction[]));
    MemberMessageAction action = MemberMessageAction.FromName(
        messages,
        Request.Form["action"]));
    return action.Perform();
}

class MoveToFolder : SpecialAction { /*...*/ }
class ExportXml : SpecialAction { /*...*/ }
class ExportCsv : SpecialAction { /*...*/ }

class Delete : MemberMessageAction { /*...*/ }
class MarkRead : MemberMessageAction { /*...*/ }
class MarkUnRead : MemberMessageAction { /*...*/ }

abstract class MemberMessageAction {
    protected MemberMessageSaveAction[] messages;
    public MemberMessageAction(MemberMessageSaveAction[] ms) { messages = ms; }
    public abstract ActionResult Perform();
    public static MemberMessageAction FromName(MemberMessageSaveAction[] ms, string action) {
        // stupid code
        // return new Delete(ms);
    }
}

abstract class SpecialAction : MemberMessageAction {
    protected IList<MemberMessage> items;
    public SpecialAction(MemberMessageSaveAction[] ms) : base(ms) {
        // Build items
    }
}

Now you can easily factor the code.

Answer 3

hmm maybe most of this belongs in my service layer?

Answer 4

I think you can also leverage the mvc framework for most of your code. Correct me if I'm wrong because I'm gonna make a few assumptions about your classes because I can't deduct it from your post.
My assumptions:

• Request.Form["action"] is a single value selectbox
• Request.Form["value"] is a multy value selectbox
• action is the kind of action you want to be taken on all the messages
• message is the list of values that should go with the action

I would try to leverage the framework's functionality where possible

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult SaveMemberAction(SelectList selectedMessages, MemberMessageAction actionType){
     //Refactors mentioned by others        
}

If you then give your inputs in your Html the correct name (in my example that would be selectedMessages and actionType) the first few rules become unnessecary.

If the default modelBinder cannot help you, you might want to consider putting the parsing logic in a custom modelbinder. You can search SO for posts about it.

As a side note: you might want to reconsider your variable namings. "action" might be confusing with MVC's action (like in ActionResult) and MemberMessageSaveAction might look like it's a value of MemberMessageAction enum. Just a thought.

Answer 5

well its not a select box but check boxes listed next to the messages.

I have not tried but can asp.net mvc do Action(int[] values) like monorail can?

otherwise I will just do it that way instead of using the NameValueDeserializer from mvccontrib.

thanks, mike

Answer 6

I came up with this.

 [AcceptVerbs(HttpVerbs.Post)]
 public ActionResult Update(MemberMessageUpdate[] messages, MemberMessage.Action action)
 {
  var actions = new List<MemberMessage.Action>
     {
      MemberMessage.Action.MoveToFolder,
      MemberMessage.Action.ExportCsv,
      MemberMessage.Action.ExportText,
      MemberMessage.Action.ExportText
     };

  if (actions.Contains(action))
  {
   IList<MemberMessage> items = new List<MemberMessage>();

   for (var i = 0; i < messages.Length; i++)
   {
    if (messages[i].Selected == false)
    {
     continue;
    }

    items.Add(MessageRepository.FetchByID(messages[i].ID));
   }

   if (action == MemberMessage.Action.MoveToFolder)
   {
    var data = new MessageMoveViewData
             {
              Messages = items
             };

    return View("move", data);
   }

   return new MessageDownloadResult(Identity.ID, items, action);
  }

  MessageRepository.Update(messages, action);

  return Url<MessageController>(c => c.Index(null, null, null, null)).Redirect();
 }

Answer 7

I don't like

MessageRepository.FetchByID(messages[i].ID)

this will make messages.Length (selected) queries to the database. I think you need to store your messages in ViewData, perform a filtering and pass them to Update() without the need to requery your database.

Answer 8

well I actually do need to do that to export them as a download for xml, csv or text. I really don't need to do it for moving them but it is nice to see what messages you are about to move.