2

Having the following classes:

Person.cs

namespace Project.Domain
{
    public class Person
    {
        public int Id { get; set; }

        public string Name { get; set; }

        public decimal Money { get; set; }

        public decimal CreditScore { get; set; }

        public Person()
        {
            Money = 0;
            CreditScore = 0;
        }

        public void RandomCreditScore()
        {
            CreditScore = Random.GetRandomDecimal();
        }
    }
}

PersonService.cs

namespace Project.Service
{
    public class PersonService
    {
        public void Save(Person person)
        {
            personRepository.Save(person);
        }

        public void Save(PersonViewModel personViewModel)
        {
            Person person = PersonAssembler.Assemble(personViewModel);

            if (personViewModel.randomCreditScore)
                person.RandomCreditScore();

            personRepository.Save(person);
        }

    }
}

PersonController

namespace Project.Controllers
{
    public class PersonController
    {

        public ActionResult Create(PersonViewModel personViewModel)
        {
            Person person = PersonAssembler.Assemble(personViewModel);

            if (personViewModel.randomCreditScore)
                person.RandomCreditScore();

            personService.Save(person);
        }

        public ActionResult CreateAlternative(PersonViewModel personViewModel)
        {
            personService.Save(personViewModel);
        }

    }
}

And having a view in which a user can input the name, money and checkbox "set a random credit score"; which of the two controller methods would be preferred? Create or CreateAlternative?

In Create we manage the conversion from the view-model to the model in the controller and we avoid the service layer to have to know the view-model, which I think is a good thing. But on the other hand we are giving the controller the responsibility to know when to set a random credit score, given the user input.

In CreateAlternative well.. It's the opposite.

Another alternative would be to make the assembler handle the logic of when to calculate a random credit score, but I've haven't seen this much.

Is one of this alternatives a better option? Is there any added advantage of using any of them?

  • "But on the other hand we are giving the controller the responsibility to know when to set a random credit score" - not really; I mean, you are using the person view model to set all the other properties from the user input, and this is just another user-supplied value that results in a property being set; just move that code into the PersonAssembler so that it's all in one place, or, if you can, add a constructor to the Person class, pass in the randomCreditScore parameter, and move the logic there, so that you have a well-formed object right after construction. – Filip Milovanović Nov 7 '19 at 8:09
1

You can introduce dedicated type for person creation data.

public class NewPerson
{
    public Person Person { get; set; }
    public bool UseRandomCreditScore { get; set; }
}

Controller will be responsible for "mapping" viewmodel data into "business" data and pass it to the service.

public ActionResult Create(PersonViewModel personViewModel)
{
    var newPerson = new NewPerson
    {
        Person = PersonAssembler.Assemble(personViewModel),
        UseRandomCreditScore = personViewModel.randomCreditScore
    };

    personService.Save(newPerson);
}

In service

public void Save(NewPerson newPerson)
{
    var person = newPerson.Person;
    if (newPerson.UseRandomCreditScore)
    {
        person.RandomCreditScore();
    }

    personRepository.Save(person);
}

Dedicated type will "explicitly" tell others what data required for person creation.
Because viewmodel responsibility to have data prepared for the client side and it can contain some data not related to user creation.

This approach will protect business logic from possible change in the way how person displayed on the client side (viewmodel)

  • I'm not sure it's a good idea to have PersonService.Save alter observable state in the Person instance passed to it (RandomCreditScore sets CreditScore). That's a non-obvious side effect. – Filip Milovanović Nov 7 '19 at 8:17
0

Albeit a video-response is atypical (and might be voted down for no other reason), the best answer is if you watch this Robert C. Martin video on MVC (classic and for web), as his explanation will somehow click on your brain and it'll all make sense. After that point, you will just intuitively do as told (because there's no other real way to actually do it that still makes sense). Notice that MVC part starts at min 27:30 https://m.youtube.com/watch?feature=youtu.be&v=o_TH-Y78tt4&t=1650

Trying to explain it in text might feel more like an imposition rather than the one and only choice after comparing the alternatives as shown on video.

  • I might be missing something, but I don't think the video has a useful explanation for this particular question. It sure rules out what things shouldn't be done, but the question was not about those. Thank you anyways, it was nice to watch Robert C. Martin. – Maximiliano Nov 21 '19 at 14:53
  • Probably you're right. It's more about don'ts than dos. Althought, this one question gets pseudo-answered by discarding all the bad choices and understanding why, for example, you should place the logic in a single place that all views can feed from, such that all views have the same fair amount of "truth" as any other – DanielCuadra Nov 21 '19 at 23:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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