Method parameters design consideration

Question

I have a AddCustomer() that has four parameters (firName, lastName, email, companyId), like below.

public class CustomerService
    {

            public bool AddCustomer(
                    string firName, string lastName, 
                    string email, int companyId)
            {

           //logic: create company object based on companId

           //other logic including validation

           var customer = //create customer based on argument and company object 

           //save the customer

        }

    }

     public class Customer
    {
        public int Id { get; set; }

        public string FirstName { get; set; }

        public string LastName { get; set; }

        public Company Company { get; set; }

        public string EmailAddress { get; set; }

        //Other five primitive properties

    }

        public class Company
    {
        public int Id { get; set; }

        public string Name { get; set; }

    }

My Question is that should the AddCustomer's parameter be changed to Customer object, like below, considering SOLID principle. Please note that only four fields shown above are used in the method.

 public bool AddCustomer(Customer customer){         
         }

Update

If below is used:

public bool AddCustomer(Customer customer)

The issue: One of the parameter is CompanyId. Thus, creating a Customer constructor with a CompanyId as parameter might not work on all circumstances. However, without constructor, it would be confusing for AdCustomer()'s client as to what properties to assign.

Update 2

Ideally, i would like to protect invariant of entities Customer and Company by restricting property setters.

Answer 1

An answer very much depends on what the purpose and the responsibility of the CustomerService class and the Customer class is, and what they are intended to achieve.

From your question it would seem ("other logic including validation") that it is the responsibility of CustomerService to determine what constitutes a valid new Customer to be registered, whereas the Customer class itself is nothing more than a DTO without any behavior.

So consider the following hypothetical use cases: a customer's email changes; the Company the Customer works for changes; if the Company is bankrupt, the new Customer registration should be refused; if the Company produces a lot of sales for us, the Customer should be regarded as a Premium Customer. How would such cases be handled and what responsibilities are involved?

You might want to approach this differently, in the sense that you make both intent and behavior explicit, instead of having "AddCustomer", "UpdateCustomer", "DeleteCustomer" and "GetCustomer(Id)". The Customer service could be responsible for service coordination and infrastructure aspects, while the Customer class really focuses on the required domain behavior and customer related business rules.

I will outline one (a CQRS type approach) of several possible approaches to better break up responsibilities, to illustrate this:

Encode behavioral intent and decisions as Commands and Events respectively.

namespace CustomerDomain.Commands
{
    public class RegisterNewCustomer : ICommand
    {
        public RegisterNewCustomer(Guid registrationId, string firstName, string lastName, string email, int worksForCompanyId)
        {
            this.RegistrationId = registrationId;
            this.FirstName = firstName;
            // ... more fields
        }
        public readonly Guid RegistrationId;
        public readonly string FirstName;
        // ... more fields
    }
    public class ChangeCustomerEmail : ICommand
    {
        public ChangeCustomerEmail(int customerId, string newEmail)
        // ...
    }
    public class ChangeCustomerCompany : ICommand
    {
        public ChangeCustomerCompany(int customerId, int newCompanyId)
        // ...
    }
    // ... more commands
}

namespace CustomerDomain.Events
{
    public class NewCustomerWasRegistered : IEvent
    {
        public NewCustomerWasRegistered(Guid registrationId, int assignedId, bool isPremiumCustomer, string firstName /* ... other fields */)
        {
            this.RegistrationId = registrationId;
            // ...
        }
        public readonly Guid RegistrationId;
        public readonly int AssignedCustomerId;
        public readonly bool IsPremiumCustomer;
        public readonly string FirstName;
        // ...
    }
    public class CustomerRegistrationWasRefused : IEvent
    {
        public CustomerRegistrationWasRefused(Guid registrationId, string reason)
        // ...
    }
    public class CustomerEmailWasChanged : IEvent
    public class CustomerCompanyWasChanged : IEvent
    public class CustomerWasAwardedPremiumStatus : IEvent
    public class CustomerPremiumStatusWasRevoked : IEvent
}

This allows expressing intent very clearly, and including only the information that is actually needed to accomplish a specific task.

Use small and dedicated services to deal with the needs of your application domain in making decisions:

namespace CompanyIntelligenceServices
{
    public interface ICompanyIntelligenceService
    {
        CompanyIntelligenceReport GetIntelligenceReport(int companyId); 
        // ... other relevant methods.
    }

    public class CompanyIntelligenceReport
    {
        public readonly string CompanyName;
        public readonly double AccumulatedSales;
        public readonly double LastQuarterSales;
        public readonly bool IsBankrupt;
        // etc.
    }
}

Have the CustomerService implementation deal with infrastructure / coordination concerns:

public class CustomerDomainService : IDomainService
{
    private readonly Func<int> _customerIdGenerator;
    private readonly Dictionary<Type, Func<ICommand, IEnumerable<IEvent>>> _commandHandlers;
    private readonly Dictionary<int, List<IEvent>> _dataBase;
    private readonly IEventChannel _eventsChannel;
    private readonly ICompanyIntelligenceService _companyIntelligenceService;

    public CustomerDomainService(ICompanyIntelligenceService companyIntelligenceService, IEventChannel eventsChannel)
    {
        // mock database.
        var id = 1;
        _customerIdGenerator = () => id++;
        _dataBase = new Dictionary<int, List<IEvent>>(); 

        // external services and infrastructure.
        _companyIntelligenceService = companyIntelligenceService;
        _eventsChannel = eventsChannel;

        // command handler wiring.
        _commandHandlers = new Dictionary<Type,Func<ICommand,IEnumerable<IEvent>>>();
        SetHandlerFor<RegisterNewCustomerCommand>(cmd => HandleCommandFor(-1,
            (id, cust) => cust.Register(id, cmd, ReportFor(cmd.WorksForCompanyId))));
        SetHandlerFor<ChangeCustomerEmail>(cmd => HandleCommandFor(cmd.CustomerId, 
            (id, cust) => cust.ChangeEmail(cmd.NewEmail)));
        SetHandlerFor<ChangeCustomerCompany>(cmd => HandleCommandFor(cmd.CustomerId,
            (id, cust) => cust.ChangeCompany(cmd.NewCompanyId, ReportFor(cmd.NewCompanyId))));
    }
    public void PerformCommand(ICommand cmd)
    {
        var commandHandler = _commandHandlers[cmd.GetType()]; 
        var resultingEvents = commandHandler(cmd);
        foreach (var evt in resultingEvents)
            _eventsChannel.Publish(evt);
    }
    private IEnumerable<IEvent> HandleCommandFor(int customerId, Func<int, Customer, IEnumerable<IEvent>> handler)
    {
        if (customerId <= 0)
            customerId = _customerIdGenerator();
        var events = handler(LoadCustomer(customerId));
        SaveCustomer(customerId, events);
        return events;
    }
    private void SetHandlerFor<TCommand>(Func<TCommand, IEnumerable<IEvent>> handler)
    {
        _commandHandlers[typeof(TCommand)] = cmd => handler((TCommand)cmd);
    }
    private CompanyIntelligenceReport ReportFor(int companyId)
    {
        return _companyIntelligenceService.GetIntelligenceReport(companyId);
    }
    private Customer LoadCustomer(int customerId)
    { 
        var currentHistoricalEvents = new List<IEvent>();
        _dataBase.TryGetValue(customerId, out currentHistoricalEvents);
        return new Customer(currentHistoricalEvents);
    }
    private void SaveCustomer(int customerId, IEnumerable<IEvent> newEvents)
    {
        List<IEvent> currentEventHistory;
        if (!_dataBase.TryGetValue(customerId, out currentEventHistory))
            _dataBase[customerId] = currentEventHistory = new List<IEvent>();
        currentEventHistory.AddRange(newEvents);
    }
}

And then that allows you to really focus on the required behavior, business rules and decisions for the Customer class, maintaining only the state needed to perform decisions.

internal class Customer
{
    private int _id; 
    private bool _isRegistered;
    private bool _isPremium;
    private bool _canOrderProducts;

    public Customer(IEnumerable<IEvent> eventHistory)
    {
        foreach (var evt in eventHistory)
            ApplyEvent(evt);
    }

    public IEnumerable<IEvent> Register(int id, RegisterNewCustomerCommand cmd, CompanyIntelligenceReport report)
    {
        if (report.IsBankrupt)
            yield return ApplyEvent(new CustomerRegistrationWasRefused(cmd.RegistrationId, "Customer's company is bankrupt"));
        var isPremium = IsPremiumCompany(report);
        yield return ApplyEvent(new NewCustomerWasRegistered(cmd.RegistrationId, id, isPremium, cmd.FirstName, cmd.LastName, cmd.Email, cmd.WorksForCompanyID));
    }
    public IEnumerable<IEvent> ChangeEmail(string newEmailAddress)
    {
        EnsureIsRegistered("change email");
        yield return ApplyEvent(new CustomerEmailWasChanged(_id, newEmailAddress));
    }
    public IEnumerable<IEvent> ChangeCompany(int newCompanyId, CompanyIntelligenceReport report)
    {
        EnsureIsRegistered("change company");
        var isPremiumCompany = IsPremiumCompany(report);
        if (!_isPremium && isPremiumCompany)
            yield return ApplyEvent(new CustomerWasAwardedPremiumStatus(_id));
        else 
        {
            if (_isPremium && !isPremiumCompany)
                yield return ApplyEvent(new CustomerPremiumStatusRevoked(_id, "Customer changed workplace to a non-premium company"));
            if (report.IsBankrupt)
                yield return ApplyEvent(new CustomerLostBuyingCapability(_id, "Customer changed workplace to a bankrupt company"));
        }
    }
    // ... handlers for other commands
    private bool IsPremiumCompany(CompanyIntelligenceReport report)
    {
        return !report.IsBankrupt && 
            (report.AccumulatedSales > 1000000 || report.LastQuarterSales > 10000);
    }
    private void EnsureIsRegistered(string forAction)
    {
        if (_isRegistered)
            throw new DomainException(string.Format("Cannot {0} for an unregistered customer", forAction));
    }
    private IEvent ApplyEvent(IEvent evt)
    {
        // build up only the status needed to take domain/business decisions.
        // instead of if/then/else, event hander wiring could be used.
        if (evt is NewCustomerWasRegistered)
        {
            _isPremium = evt.IsPremiumCustomer;
            _isRegistered = true;
            _canOrderProducts = true;
        }
        else if (evt is CustomerRegistrationWasRefused)
            _isRegistered = false;
        else if (evt is CustomerWasAwardedPremiumStatus)
            _isPremium = true;
        else if (evt is CustomerPremiumStatusRevoked)
            _isPremium = false;
        else if (evt is CustomerLostBuyingCapability)
            _canOrderProducts = false;
        return evt;
    }
}

An added benefit is that the Customer class in this case is completely isolated from any infrastructure concerns can be easily tested for correct behavior and the customer domain module can be easily changed or extended to accommodate new requirements without breaking existing clients.

Answer 2

How about using the builder pattern resulting in code somewhat like this:

var customer = new CustomerBuilder()
                     .firstName("John")
                     .lastName("Doe")
                     .email("john.doe@example.com")
                     .companyId(6)
                     .createCustomer();

customerService.AddCustomer(customer);

Then you can have your builder class handle looking up company objects when createCustomer is called and the order of parameters no longer matters and you have a convenient place to put logic to choose sensible defaults.

This also gives you a convenient location for validation logic so you can't get an invalid instance of Customer in the first place.

Or another possible way would be to have AddCustomer return a command object so your client code could do this:

customerService.AddCustomer()
    .firstName("John")
    .lastName("Doe")
    .email("john.doe@example.com")
    .companyId(6)
    .execute();

Answer 3

yes.... if its valid to create a customer with those 4 properties.... ideally you'd have a constructor with those 4. that way the create responsibility lives with the customer object and Customer Service doesn't need to know about it, it just deals with "Customers".