SRP applied to a workflow example: how to structure the classes in a sensible way
Asked Answered
D

1

1

I have a problem with deciding about class responsibilities.
I have 3 html-forms:

  1. For each form there is a html template containing some text and a marker for the form to be included
  2. Each form needs to be validated, if there is an error the template and the form from (1) needs to redisplayed, together with some error messages. Only some fields are common across the different forms.
  3. If there are no errors a result message needs to be mailed. For each form there is a result mail template.

I find it very hard to decide on a good class scheme for this problem. One possibility is to separating classes by functionality

  • CheckFormData: checking form data
  • DisplayForm: display form with/without errors (or separate this too?)
  • EmailForm: emailform.

I am uncertain about this. Knowledge about the fields of one particular form are dispersed accross the various classes.

There is some workflow. Maybe I should also have a workflow class:

class FormSubmitWorkFlow
{
   function start() 
   {
     $this->displayForm->render();
   }

   function processFormData($data)
   {
      $this->checkForm->setData($data);
      if (!$this->checkForm->isValid())    {

         $errors = $this->checkForm->getErrors();
         $this->displayForm->setData($data)->setErrors($errors)->render();
      } else {
         $this->emailForm->setData($data)->email();
      }
   }

   function setDisplayForm(DisplayForm $df)
   {
      $this->displayForm = $df;
   }

   function setCheckForm(CheckForm $cf)
   {
      $this->checkForm = $cf;
   }

   function setEmailForm(CheckForm $ef)
   {
      $this->emailForm = $ef;
   }
}

For each form type (remember, there are 3 of them) I would need a

  1. CheckForm,
  2. EmailForm and
  3. DisplayForm class.

3*3 = 9 classes + 3 base classes = 12 classes.
Also, you want to inject the right CheckForm-subclass and EmailForm-subclass into the workflow, they all need to be of the same form type. Maybe we need to create a FormWorkFlowFactory for this. This adds up to 13 classes.

Now I got the feeling I am doing something horribly wrong. If I had FormSubmitWorkFlow as a Template Method class, I could just create 3 subclasses, but each subclass would mix different responsibilities.

How could you improve this, and could you motivate your answer, i.e. what method leads you to your answer?


edit: although the only current answer is useful, it would be nice to see some votes from people that agree with it, or I would like to hear better solutions from the community. I am the only one who upvoted this answer. This question could use more input, so feel free to provide so :-)

Dejecta answered 24/11, 2009 at 20:17 Comment(3)
One piece of advice that might or might not help is to change the question around. I notice that it only has 25 views. I think that is partly because of the tags you chose, partly how the title is worded and the fact the question is somewhat hard to understand. What I'd suggest is to add tags that get more views like oo design or things like that. Also try to make a more eye catching title like: 'Am I creating too many classes when trying to apply SRP?' Then try to reduce your question to a much smaller size so more people will read it. It takes some work to condense it but it's worth it.Opsonin
I meant to say to repost the question, just differently.Opsonin
Thanks for your suggestion. Will repost it in a more attractive way. A bit of shame since questions ought to be detailed and focussed. Thanks for your support!Dejecta
O
1

I'm not sure if this will answer your question or not but obviously the goal of SRP is to write your code so that if you have to change something, it's for one reason only. Like if your car had SRP then there wouldn't be a class that adjusted the temperature and also put the windows up and down. That would violate the principle. In this case you seem to be doing it right. Yes it's a lot of classes but the alternative is a lot of confusion. Unless there was a way that you could create a single class that would validate the forms (which really should be possible depending on what sort of verification you need).

If you created a verification class where you could just add a chain of expected values and see if they matched then you would probably have more overall classes but I think that it would be far less coupled. Let me know if I understood you correctly or not.

Opsonin answered 25/11, 2009 at 7:50 Comment(4)
Thanks for your attention! So if I understood you correctly you would vote for the 12 classes + 1 one factory? A generic validation class might not work in case you have logic like if field1 > 12 then field2 must be between 3 and 12, else if... Or is your chain meant to handle this too? I could pack this into one class but then I have something like if ($this->formType === self::FORM_TYPE_ONE) etc. I thought this is not beautifull?Dejecta
I think personally it would be best to have a class that would handle that type of input as well for example validator.test(field1 > 12); This way possibly you could just have the validator pass itself or certain messages to the error controller. I would definitely want to inject my verificator (or a class that is wrapping it) into the error controller. You want things as separate as possible and that to me seems to be safest bet.Opsonin
I do not fully understand the line validator.test(field1 > 12). This will evaluate to validator.test(<bool>). How does this fit within the whole picture? Normally I only hit the error controller if there is something wrong which I can't handle in the current controller. Handling form entry errors is the duty of the validator (it injects the error in the form). I think you will agree this place makes the most sense?Dejecta
Ah okay I see what you are doing. Yeah I think what you said makes sense. It sounds to me at least like a solid way of doing itOpsonin

© 2022 - 2024 — McMap. All rights reserved.