current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

I want to keep things DRY, so I'm trying to figure out: Is it possible to remove repetitive parameters and code from ASP.NET MVC Controller actions? eg given these Actions:

public ActionResult List(string key, string signature) { GetSignature(key); // etc } 
public ActionResult Add(string key, string signature) { GetSignature(key); // etc } 
public ActionResult Update(string key, int id, string signature) { GetSignature(key, id.ToString()); // etc} 
public ActionResult Delete(string key, int id, string signature) { GetSignature(key, id.ToString()); // etc} 
public ActionResult Action1(string key, string x, string y, string z, string signature) { GetSignature(key, x, y, z);//etc}

Is there any way to refactor out the repetitive key and signature parameters, and the call to GetSignature()?

There are two obstacles to refactoring I can think of:

  1. It's MVC so the Actions are the first port of call - it may not be possible to have a refactored method that is called before the Action.
  2. The GetSignature() method is called using all of the Action's parameters, so it may be difficult to refactor.
share|improve this question
up vote 3 down vote

I like the direction Scott started but would recommend to take it another layer down so that the GenerateSignature method does not have to do discovery with the thing object (Do I have an Id or do I have string X/Y/Z or do I only have Key/Signature, etc).

public class MySignatureCreateRead
{
    public string Key { get; set; }
    public string Signature { get; set; }
}

public class MySignatureUpdateDelete : MySignatureCreateRead
{
    public int Id { get; set; }
}

public class MySignatureAction : MySignatureCreateRead
{
    public string X { get; set; }
    public string Y { get; set; }
    public string Z { get; set; }
}

Which puts the ActionResults like:

public ActionResult List(MySignatureCreateRead signature) { GetSignature(signature); /* etc */ }
public ActionResult Add(MySignatureCreateRead signature) { GetSignature(signature); /* etc */ }
public ActionResult Update(MySignatureUpdateDelete signature) { GetSignature(signature); /* etc */ }
public ActionResult Delete(MySignatureUpdateDelete signature) { GetSignature(signature); /* etc */ }
public ActionResult Action1(MySignatureAction signature) { GetSignature(signature); /* etc */ }

This also gives you objects to strongly bind views to.

share|improve this answer
up vote 1 down vote

Of course, my suggestion would be to encapsulate your properties in a class:

public class MySignatureThing
{
  public string Key {get;set;}
  public int Id {get;set;}
  public string Signature {get;set;}
  public string X {get;set;}
  public string Y {get;set;}
  public string Z {get;set;}
}

Then your action methods change to this:

public void Add(MySignatureThing thing) { GenerateSignature(thing); }
share|improve this answer
up vote 0 down vote

You could create a factory to create ActionResults, and make the key and signature members of the factory. Unless you are reusing the same key and signature a lot of the time, it's likely to look like:

myList = new ActionResultsFactory(key, signature).List();

Which hardly seems better than

myList = new List(key, signature);

but maybe it has value in some situations.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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