Solved

how to i transform my below class utilizing the reflection.

Posted on 2015-02-17
14
104 Views
Last Modified: 2015-02-18
Hi,

For one of the projects that I am working on that does form submissions, i need to make an enhancement to a class but i am a junior developer and i am having hard time how to think and make this class the most effective one and utilizing the abstraction as well.. my manager looked at the class and asked to make the class abstract and using reflection it will decide which method to use he said and left so i need to figure out what to do .  Below is the class and i need to make an abstract class and empty the function etc.

Could you help me by telling step by step what i can do to this class to make this an effective one. i can see the role of the abstraction here but what he meant with reflection it did not make any sense to me... how should i go with this class... your help is much appreciated


public class ProcessActions
    {
        private ActionSendFormEmail _mailAction;
        private ActionHttpPostDataForCzAndSk _czskWebService;
        private readonly Cache _Cache;
        private PreOrderFormPostData _preOrderService;


        public ProcessActions ()
        {
            _mailAction = new ActionSendFormEmail();
            _czskWebService = new ActionHttpPostDataForCzAndSk();
            _preOrderService = new PreOrderFormPostData();
            _Cache = new Cache();
        }
        public void DoProcess(string culture, Submission submission, SubmissionResponse submissionResponse, HttpRequest request)
        {
            #region PostSubmissionSuccessfulActions

            if (!string.IsNullOrEmpty(HttpContext.Current.Request.Form["PostSubmissionAction"]))
            {
                var requiredActions = HttpContext.Current.Request.Form["PostSubmissionAction"];
                
                //dynamic requiredActionList = JsonConvert.DeserializeObject(jsn);
                var ser = new JavaScriptSerializer();
                var cmnds = ser.Deserialize<List<ActionBehaviourJson>>(requiredActions);
                Dealer dealer = null;
                if (!string.IsNullOrEmpty(submission.UserData.LocalDealerId))
                    dealer = _Cache.GetDealerById(submission.UserData.LocalDealerId);
                foreach (var action in cmnds)
                {
                    switch (action.ActionIdentifier.ToLowerInvariant())
                    {
                        case EventActionAttributes.UserEmailAction:
                            _mailAction.Do(submission, submissionResponse, null, dealer, action.Params);
                            break;
                        case EventActionAttributes.DealerEmailAction:
                        case EventActionAttributes.OtherEmailAction:                         
                            _mailAction.Do(submission, submissionResponse, null, dealer, action.Params,
                                string.Equals(action.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                                    StringComparison.InvariantCultureIgnoreCase)
                                    ? EmailManagerType.Dealer
                                    : string.Equals(action.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                                        StringComparison.InvariantCultureIgnoreCase)
                                        ? EmailManagerType.Other
                                        : EmailManagerType.Customer);
                            break;
                        case EventActionAttributes.CzSkWebserviceAction:
                            _czskWebService.Do(submission);
                            break;
                        case EventActionAttributes.Preorderdata:
                            _preOrderService.Do(submission);
                            break;
                        default:
                            break;
                    }

                }

            }
          
            #endregion
        }
    }

Open in new window

0
Comment
Question by:nicedone
  • 6
  • 4
  • 4
14 Comments
 
LVL 32

Expert Comment

by:ste5an
ID: 40616183
First of all: Your names show that there is something to improve.

ProcessActions and DoProcess have a to general meaning. Thus they are they point to start your refactoring.
What kind of actions? What kind of process? The answer on these question will give us some clues about what we can do, like using design patterns.

The thing I see first here is Separation Of Concerns: I would seperate the deserialization from the rest. Like an abstract action class.

The second thing which may be an option is Chain of Responsibilty to get rid of that ugly switch block.
0
 
LVL 22

Expert Comment

by:ambience
ID: 40616194
For one it is always the right attitude to openly discuss matters of confusion with your peers and superiors. Why cant you just discuss these problems with real developers/managers in your team? It is a healthy attitude to seek guidance, and being a junior developer others have to be supportive. Dont be afraid of making stupid statement, wasting lot of time and coming up short on expectations is going to be worse than asking a lot of question right at the start.

Probably what your manager meant by reflection is that the deserialized list

var requiredActions = HttpContext.Current.Request.Form["PostSubmissionAction"];

has a one to one mapping with class action class names. Like for example if the deserialized ActionBehaviourJson has and identifier "UserEmailAction" then you can create an object of type say "UserEmailAction" and have it handle. OR maybe only polymorphism was intended especially since "abstract" was also mentioned.

There is a select/case statement in your code where you dispatch actions to different objects, based on the type of object (or so it appears). Such code is often indicative of a place for polymorphic design.

You can start by writing a BaseAction class and different specializations of it, like

// Note that is better to use interface (don't use inheritance unless absolutely necessary)
interface IAction {
    void Do(......);
}

class UserEmailAction : IAction {
    public override void Do(......) {
    }
}

Open in new window


You can then move the action specific code to the corresponding implementations and make the ProcessAction class a simple and generic dispatcher. Note that the Cache is only used by the EmailAction, since it needs a dealer. It should therefore make sense to move the cache and related stuff to the actions that need it. Keep the Action interface as generic as possible.

Make sure you get that working and well-designed without taking into account reflection. For creating action handler objects you can create a Factory like

interface IActionFactory {
   IAction CreateAction(int identifier);
}

class StaticActionFactory : IActionFactory {
   public IAction CreateAction(int identifier) {
        select(identifer) { ....... }
   }
}

Open in new window


You can then create another Factory that creates the Actions using reflection or better using a Composition Container, if your application uses Dependency Injection.
0
 

Author Comment

by:nicedone
ID: 40616215
Hi , thank you for your comments, In terms of asking managers, unfortunately managers are too busy to deal with others' work and they are only called managers but doing soft. dev. as developers too so it is not easy to find their time to ask question ,i still ask but they generally do not go throughly with your questions unfortunately and if they see that it will take for them to tell you, it is ok for them to just dont do the work or leave it in its worst case.

As the problem is concerned...

if i go one by one before getting confused and trying to get rid of switch block and make this class just a dispatcher. As you can see, "Do" funtion in czskWebService and _czskWebService takes only submission as parameter but _mailAction's "Do" has different number,types of parameters so I wonder how creating an interface like in below would help to override Do method since it has different signatures of method "Do"

Can you clarify how i can implement IAction with different signatured Do functions?

// Note that is better to use interface (don't use inheritance unless absolutely necessary)
interface IAction {
    void Do(......);
}

class UserEmailAction : IAction {
    public override void Do(......) {
    }

Open in new window


 switch (action.ActionIdentifier.ToLowerInvariant())
                    {
                        case EventActionAttributes.UserEmailAction:
                            _mailAction.Do(submission, submissionResponse, null, dealer, action.Params);
                            break;
                        case EventActionAttributes.DealerEmailAction:
                        case EventActionAttributes.OtherEmailAction:                         
                            _mailAction.Do(submission, submissionResponse, null, dealer, action.Params,
                                string.Equals(action.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                                    StringComparison.InvariantCultureIgnoreCase)
                                    ? EmailManagerType.Dealer
                                    : string.Equals(action.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                                        StringComparison.InvariantCultureIgnoreCase)
                                        ? EmailManagerType.Other
                                        : EmailManagerType.Customer);
                            break;
                        case EventActionAttributes.CzSkWebserviceAction:
                            _czskWebService.Do(submission);
                            break;
                        case EventActionAttributes.Preorderdata:
                            _preOrderService.Do(submission);
                            break;
                        default:
                            break;
                    }

Open in new window

0
 
LVL 22

Expert Comment

by:ambience
ID: 40616240
Yep, that was logically the next question you should have had : )

See what makes more sense for an Action (think about the domain only), dont consider that some actions may not need all parameters. For example, it may make sense to pass the following to all actions

submission, // this appears to be needed by all
submissionResponse,  // dont know the purpose of it, but seems like a logical need?
action.Params // again appears logical to pass this since this is extracted from the request and you dont want to have an Action mess with the Request again

Another pattern that often helps in these cases is to use a Context object and bundle everything in it http://www.javagyan.com/tutorials/corej2eepatterns/presentation-tier-patterns/context-object. You can then pass just the context to every action. for example

class ActionContext {

public Submission Submission {get; set;}
public Parameters Parameters {get; set;}
}

This is so very often used in the ASP MVC framework, e.g. HttpActionContext, ControllerContext, RequestContext

NOTE: I would recommend against the Context unless it is very necessary to adopt it. A Context often ends up becoming a mess since there is the desire to keep adding stuff to it, which is not always the right thing to do in every situation.
0
 
LVL 32

Expert Comment

by:ste5an
ID: 40616258
@ambience: Now you were faster :)

As long as you don't get a better grip on this, a context object is a good idea.
0
 

Author Comment

by:nicedone
ID: 40616278
Thanks so much, i will implement context object after i am done with the IAction... so i have implemented an interface with Do, and made all "Do"s have a common signature not my switch statement looks as below
Although i though i have made an improvement, it does not look as such... still i create an IAction object and call the method on it, and still relying on the switch. block , from the code below what should i further do to get rid of swich or improve below you think ?


   switch (action.ActionIdentifier.ToLowerInvariant())
                    {
                        case EventActionAttributes.UserEmailAction:
                            IAction act=new ActionSendFormEmail();
                            act.Do(submission, submissionResponse, null, dealer, action.Params);
                         //   _mailAction.Do(submission, submissionResponse, null, dealer, action.Params);
                            break;
                        case EventActionAttributes.DealerEmailAction:
                        case EventActionAttributes.OtherEmailAction:
                           IAction actSendEmail=new ActionSendFormEmail();
                           actSendEmail.Do(submission, submissionResponse, null, dealer, action.Params,
                               string.Equals(action.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                                   StringComparison.InvariantCultureIgnoreCase)
                                   ? EmailType.Dealer
                                   : string.Equals(action.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                                       StringComparison.InvariantCultureIgnoreCase)
                                       ? EmailType.Other
                                       : EmailType.Customer);
                            //_mailAction.Do(submission, submissionResponse, null, dealer, act.Params,
                            //    string.Equals(act.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                            //        StringComparison.InvariantCultureIgnoreCase)
                            //        ? EmailType.Dealer
                            //        : string.Equals(act.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                            //            StringComparison.InvariantCultureIgnoreCase)
                            //            ? EmailType.Other
                            //            : EmailType.Customer);
                            break;
                        case EventActionAttributes.CzSkWebserviceAction:
                            IAction actCzSkWebserviceAction=new ActionHttpPostDataForCzAndSk();
                            actCzSkWebserviceAction.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            //_czskWebService.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            break;
                        case EventActionAttributes.Preorderdata:
                            IAction actPreorderdata=new PreOrderFormPostData();
                            actPreorderdata.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            //_preOrderService.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            break;
                        default:
                            break;
                    }

Open in new window

0
 

Author Comment

by:nicedone
ID: 40616287
i have made one more improvement but still switch is living there with me.. :(


 IAction act;
                foreach (var action in cmnds)
                {
                    switch (action.ActionIdentifier.ToLowerInvariant())
                    {
                       
                        case EventActionAttributes.UserEmailAction:
                            act=new ActionSendFormEmail();
                            act.Do(submission, submissionResponse, null, dealer, action.Params);
                         //   _mailAction.Do(submission, submissionResponse, null, dealer, action.Params);
                            break;
                        case EventActionAttributes.DealerEmailAction:
                        case EventActionAttributes.OtherEmailAction:
                           act=new ActionSendFormEmail();
                           act.Do(submission, submissionResponse, null, dealer, action.Params,
                               string.Equals(action.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                                   StringComparison.InvariantCultureIgnoreCase)
                                   ? EmailType.Dealer
                                   : string.Equals(action.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                                       StringComparison.InvariantCultureIgnoreCase)
                                       ? EmailType.Other
                                       : EmailType.Customer);
                            //_mailAction.Do(submission, submissionResponse, null, dealer, act.Params,
                            //    string.Equals(act.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                            //        StringComparison.InvariantCultureIgnoreCase)
                            //        ? EmailType.Dealer
                            //        : string.Equals(act.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                            //            StringComparison.InvariantCultureIgnoreCase)
                            //            ? EmailType.Other
                            //            : EmailType.Customer);
                            break;
                        case EventActionAttributes.CzSkWebserviceAction:
                            act=new ActionHttpPostDataForCzAndSk();
                            act.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            //_czskWebService.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            break;
                        case EventActionAttributes.Preorderdata:
                            act=new PreOrderFormPostData();
                            act.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            //_preOrderService.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            break;
                        default:
                            break;
                    }

                }

Open in new window

0
How your wiki can always stay up-to-date

Quip doubles as a “living” wiki and a project management tool that evolves with your organization. As you finish projects in Quip, the work remains, easily accessible to all team members, new and old.
- Increase transparency
- Onboard new hires faster
- Access from mobile/offline

 

Author Comment

by:nicedone
ID: 40616298
And one more improvement i carried the call to outside the switch block and only making one call now.

but again switch exists :(

     IAction act=null;
                foreach (var action in cmnds)
                {
                    switch (action.ActionIdentifier.ToLowerInvariant())
                    {
                       
                        case EventActionAttributes.UserEmailAction:
                            act=new ActionSendFormEmail();
                         //   _mailAction.Do(submission, submissionResponse, null, dealer, action.Params);
                            break;
                        case EventActionAttributes.DealerEmailAction:
                        case EventActionAttributes.OtherEmailAction:
                           act=new ActionSendFormEmail();
                            //_mailAction.Do(submission, submissionResponse, null, dealer, act.Params,
                            //    string.Equals(act.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                            //        StringComparison.InvariantCultureIgnoreCase)
                            //        ? EmailType.Dealer
                            //        : string.Equals(act.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                            //            StringComparison.InvariantCultureIgnoreCase)
                            //            ? EmailType.Other
                            //            : EmailType.Customer);
                            break;
                        case EventActionAttributes.CzSkWebserviceAction:
                            act=new ActionHttpPostDataForCzAndSk();
                            //_czskWebService.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            break;
                        case EventActionAttributes.Preorderdata:
                            act=new PreOrderFormPostData();
                            //_preOrderService.Do(submission, submissionResponse, null, dealer, action.Params, EmailType.Customer);
                            break;
                        default:
                            break;


                    }
                    if (act != null)
                    {
                        act.Do(submission, submissionResponse, null, dealer, action.Params,
                                string.Equals(action.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                                    StringComparison.InvariantCultureIgnoreCase)
                                    ? EmailType.Dealer
                                    : string.Equals(action.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                                        StringComparison.InvariantCultureIgnoreCase)
                                        ? EmailType.Other
                                        : EmailType.Customer);
                    }

Open in new window

0
 
LVL 22

Accepted Solution

by:
ambience earned 250 total points
ID: 40616309
I dont think its a good idea to pass null and dealer as parameters.

As I said earlier, Cache/dealer is required for some actions and it appears to be a business thing, something that doesnt fit in generic handling of an action dispatcher. Also, if every action is taking null, it means that parameter is not even needed.

As for the switch.

IAction action = actionFactory.CreateAction(action.ActionIdentifier.ToLowerInvariant())
action.Do()

NOTE: This is moving the switch to the ActionFactory and as such one may doubt that the switch has really been removed, but

1) it is an improvement on the Dispatcher.
2) Factory's job is to create object and as such its not as bad to have switch/case there. Polymorphism does not even apply in that context, since the decision is *before* the creation of polymorphic objects.
3) Factory can be smarter, like one that uses Reflection or the Kernel (say NInject) to create action handlers.
0
 
LVL 32

Expert Comment

by:ste5an
ID: 40616442
Just stuff the parameters into your context class. Imho there is no need for any parameter.
Also  dealer should not be a parameter.  Let the concrete action resolve it from the cache.

Your context object should consist of submission, submission response, action parameters and a reference to the cache object.

When there are any constraints on the values, then you can also add an abstract method for validating the context object before excuting the action.

btw, your're using the cache to resolve the dealer. But you don't have a fallback when the cache has no value for it.
0
 

Author Comment

by:nicedone
ID: 40616605
Hi,

Thanks again for all the information with your help;

I have created a static actionFactory class with static actionCreate method ,giving the responsibility to factory to give me an object of  "ActionSendFormEmail" or "ActionHttpPostDataForCzAndSk" or "PreOrderFormPostData" type. with the swith statement in the factory class.

Now i am working on the context class from what you are saying what i understand is

i should call as -->  act.Do( ActionContext context)    

and are you saying that context objects Submission,submissionResponse,fromEmail etc. parameter should be filled just before calling act.Do (ActionContext context)  ?

if i happen to give my all class ,it looks as below. DoProcess function is called with parameters

(string culture, Submission submission, SubmissionResponse submissionResponse, HttpRequest request)

and act.Do takes these parameters as well as some additional..

does my ActionContext look OK ?

should i fill the properties of my ActionContext inside DoProcess or the constructor of my ActionContext ?

sorry they might seem basic questions but really trying to learn and you are being great help for it.thanks again...



   public void DoProcess(string culture, Submission submission, SubmissionResponse submissionResponse, HttpRequest request)
        {
            #region PostSubmissionSuccessfulActions

            if (!string.IsNullOrEmpty(HttpContext.Current.Request.Form["PostSubmissionAction"]))
            {
                var requiredActions = HttpContext.Current.Request.Form["PostSubmissionAction"];
                
                //dynamic requiredActionList = JsonConvert.DeserializeObject(jsn);
                var ser = new JavaScriptSerializer();
                var cmnds = ser.Deserialize<List<ActionBehaviourJson>>(requiredActions);
                Dealer dealer = null;
                if (!string.IsNullOrEmpty(submission.UserData.LocalDealerId))
                    dealer = _mazdaCache.GetDealerById(submission.UserData.LocalDealerId);
                IAction act=null;
                foreach (var action in cmnds)
                {
                    act = actionFactory.createAction(action);
                    
                    if (act != null)
                    {
                        act.Do(submission, submissionResponse, null, dealer, action.Params,
                                string.Equals(action.ActionIdentifier, EventActionAttributes.DealerEmailAction,
                                    StringComparison.InvariantCultureIgnoreCase)
                                    ? EmailType.Dealer
                                    : string.Equals(action.ActionIdentifier, EventActionAttributes.OtherEmailAction,
                                        StringComparison.InvariantCultureIgnoreCase)
                                        ? EmailType.Other
                                        : EmailType.Customer);
                      
                    }

                }

            }
          
            #endregion
        }

        class ActionContext
        {

            public Submission Submission { get; set; }
            public SubmissionResponse submissionResponse { get; set; }
            public string fromEmail { get; set; }
            public Dealer dealer { get; set; }
            public ActionBehaviourJson action { get; set; }
            public EmailType emailType { get; set; }
        }

Open in new window

0
 
LVL 32

Assisted Solution

by:ste5an
ste5an earned 250 total points
ID: 40616700
Let's make this bottom-up:

1. EmailType: Is not necessary as far as I can tell. Cause its already determined by action.ActionIdentifier. Thus our action itself - the type - has its immanent email type.

2. ActionBehaviourJson
2.1. Json: Abstraction is used to seperate concerns. So don't let our action classes work with Json. Especially as we need to deserialize earlier to get the commands and the concrete classes.
2.2 ActionBehaviour: this seems to be the entire response. But your actions only need the action.Params. So I would stuff only these into the context  object.

3. fromEmail: Where does this come from?

This should be sufficent:

class ActionContext
{
    public Submission Submission { get; set; }
    public SubmissionResponse SubmissionResponse { get; set; }
    public Dealer Dealer { get; set; }
    public ActionParams Params { get; set; }
}

Open in new window


btw, in most conventions properties are CamelCased with an upper-case first letter.

Then you can invoke it like this

public void DoProcess(string culture, Submission submission, SubmissionResponse submissionResponse, HttpRequest request)
{
    var requiredActions = HttpContext.Current.Request.Form["PostSubmissionAction"];                              
    var ser = new JavaScriptSerializer();
    var cmnds = ser.Deserialize<List<ActionBehaviourJson>>(requiredActions);    
    ActionContext context = new ActionContext();
    context.Submission = submission;
    context.SubmissionResponse = submissionResponse;
    context.Dealer = (string.IsNullOrEmpty(submission.UserData.LocalDealerId)) ? null : _mazdaCache.GetDealerById(submission.UserData.LocalDealerId);        
    foreach (var action in cmnds)
    {
        context.Params = action.Params;
        IAction act = actionFactory.createAction(action);                    
        act.Do(context);                                      
    }    
}

Open in new window


btw, as we always want an action, it's imho okay, that the factory raises an exception, when it cannot resolve the action identifier. Thus we no additional test on act != null.
0
 
LVL 22

Expert Comment

by:ambience
ID: 40616800
Totally agreed with ste5an, and would add the following.

1 - No need to include dealer (and/or Cache) in context. This is only used by EmailAction, move it there. The question you should be asking, is it OK to create a Cache in the EmailAction? Only if the answer is No, should you consider keeping it in context - though it maybe possible to fetch it from elsewhere.

Moreover, if the Cache has read-through (reads the Dealer from a Datasource upon a Miss) you incur some CPU penalty for all actions, when its not even needed. Not to mention how it could interfere with other caching semantics like Expirations of dealers.


2 - actionFactory is supposed to be an interface (as given in earlier comments). A static class with static method, even though is an improvement but defeats the original purpose of introducing the factory - being able to use reflection or other mechanism for creating actions.

To sum up some of the suggestions as a general idea: Keep the bare minimum information in the context, otherwise it would become a mess quickly.
0
 

Author Comment

by:nicedone
ID: 40616813
Thank you very much for your helps ,i appreciate it , now it looks much better, i believe the challenge and the task for me now is to make the factory class smart and make use of the reflection a bit.

Thanks...
0

Featured Post

How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

Join & Write a Comment

Problem Hi all,    While many today have fast Internet connection, there are many still who do not, or are connecting through devices with a slower connect, so light web pages and fast load times are still popular.    If your ASP.NET page …
Calculating holidays and working days is a function that is often needed yet it is not one found within the Framework. This article presents one approach to building a working-day calculator for use in .NET.
It is a freely distributed piece of software for such tasks as photo retouching, image composition and image authoring. It works on many operating systems, in many languages.
Get a first impression of how PRTG looks and learn how it works.   This video is a short introduction to PRTG, as an initial overview or as a quick start for new PRTG users.

746 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

8 Experts available now in Live!

Get 1:1 Help Now