Asp.net mvc related, basically a refactoring 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();
}
0
source to share
5 answers
I think you can also use the mvc framework for most of your code. Correct me if I am wrong because I will make some assumptions about your classes, because I cannot deduct it from your post.
My assumptions:
- Request.Form ["action"] is one selectbox value
- Request.Form ["value"] is the plural selectbox value
- an action is an action that you want to use in all messages.
- message is a list of values to come with the action
I would try to use the capabilities of the framework where possible.
[AcceptVerbs(HttpVerbs.Post)]
public ActionResult SaveMemberAction(SelectList selectedMessages, MemberMessageAction actionType){
//Refactors mentioned by others
}
If you then pass your inputs in your HTML to the correct name (in my example, which will be the Messages and actionType selected), the first few rules become awkward.
If the default modelBinder can't help you, you might need to consider the parsing logic in the custom modelbinder object. You can find SO for posts on this.
As a side note, you may want to revise your names. "action" can be confusing with MVC action (for example, in ActionResult), and MemberMessageSaveAction can look like the value of the MemberMessageAction enumeration. Just a thought.
+3
source to share
The first step is to create different methods for each action.
The next step is to remove the negative logic.
The result is something like the following:
[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(...);
}
}
+2
source to share
Include MemberMessageAction in a class that has a virtual function Run.
For your special actions, group the common execution 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 identify the code.
+2
source to share
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();
}
0
source to share