3
\$\begingroup\$

I have a list of contacts, classified into categories. Each category is represented with a different class.

Base class:

class Contact
{
    public ushort Id { get; private set; }
    public Phone Phone { get; private set; }

    public Contact(ushort id, Phone phone)
    {
        Id = id;
        Phone = phone;
    }
}

And here are the categories:

PrivateServiceProvider (meaning - private person):

class PrivateServiceProvider : Contact
{
    public string FirstName { get; private set; }
    public string LastName { get; private set; }
    public ServiceField ServiceField { get; private set; }

    public PrivateServiceProvider(ushort id, string firstName, string lastName,
                                  ServiceField serviceField, Phone phone)
        : base(id, phone)
    {
        FirstName = firstName;
        LastName = lastName;
        ServiceField = serviceField;
    }
}

BusinessServiceProvider:

class BusinessServiceProvider : Contact
{
    public string Name { get; private set; }
    public ServiceField ServiceField { get; private set; }
    public Email Email { get; private set; }

    public BusinessServiceProvider(ushort id, string name,
                                   ServiceField serviceField, Phone phone, Email email)
        : base(id, phone)
    {
        Name = name;
        ServiceField = serviceField;
        Email = email;
    }
}

PrivateGenericContact:

class PrivateGenericContact : Contact
{
    public string FirstName { get; private set; }
    public string LastName { get; private set; }

    public PrivateGenericContact(ushort id, string firstName, string lastName, Phone phone)
        : base(id, phone)
    {
        FirstName = firstName;
        LastName = lastName;
    }
}

BusinessGenericContact:

class BusinessGenericContact : Contact
{
    public string Name { get; private set; }
    public Email Email { get; private set; }

    public BusinessServiceProvider(ushort id, string name, Phone phone, Email email)
        : base(id, phone)
    {
        Name = name;
        Email = email;
    }
}

It works, but how could I improve it1?

Also, it drives me CRAZY that the names of the classes are so lengthy. I think it's a smell but I am not sure what to do about it. If some day I decide to create another specification of Contact, let's say - for people who lives in the city versus in the country2, will I call the class CountryPrivateServiceProvider? It's NUTS.

An example of use is when the user creates a contact. So he chooses the type of contact and inserts all of the information into a form (a View - I am using MVP). Then the View notify the Presenter that the user pressed the "Save" and creates an object of the selected type. Then the Presenter ask the Model to insert the information into the database.

How can I improve my code, both generally and specifically to this naming problem?


1. They are deliberately immutable.

2. Apparently, I am not good with examples.

\$\endgroup\$
5
  • \$\begingroup\$ Take out the private set; and see if it still works \$\endgroup\$
    – paparazzo
    Commented Aug 11, 2017 at 13:02
  • \$\begingroup\$ @Paparazzi The code works... The setter is private for a reason, that is what makes the objects immutable. \$\endgroup\$ Commented Aug 11, 2017 at 13:06
  • 1
    \$\begingroup\$ It is also private if there is no set; So the code works, the purpose of posting here is to make it better. \$\endgroup\$
    – paparazzo
    Commented Aug 11, 2017 at 13:08
  • \$\begingroup\$ I would say your ServiceProvider could rather be using the GenericContact as a baseclass. I would rather be unsure if you want such a hard differentation between these classes. Depending on specs that might change these might be classes that will be changed quite often. Also, I don't see how classes that should be editable by a user should be immutable, it seems like a contradition \$\endgroup\$
    – Icepickle
    Commented Aug 11, 2017 at 13:11
  • \$\begingroup\$ ushort as a unique identifier type has a bit of a smell. Just stick with int unless there's an obvious overriding concern (such as the DB is explicitly absolutely needing a 16-bit unsigned integer). \$\endgroup\$ Commented Aug 18, 2017 at 21:31

3 Answers 3

1
\$\begingroup\$

I find those classes should but just simple POCO classes with get/set properties. They could sit either inside a DTO or Model namespace or could have a Dto suffix and simple names like Person or PrivatePerson or BusinessPerson. They are not service providers as you call them.

However, you do want to differentiate between a PrivateCountry and a BusinessCountry. You could do this by adding the type of operation you need them for, like PrivateCountryForAddingDto or BusinessCountryForUpdatingDto. With this pattern you will have a few more models but the good thing is that each command will have its own independent model that can have its own validation logic. A model for adding does not necessarily have to have the same requirements as a model for updating. Some fields might be optional in one case and mandatory in another case.

\$\endgroup\$
1
\$\begingroup\$

My two cents; Lengthy class names are not necessarily a code smell, as long as they are descriptive.

I found the Ubiquitous Language in Domain Driven Design an eyeopener in how to structure and name my classes.

The link I provided describes in overview what the Ubiquitous Language is. Google some more to get a good idea of how it works.

\$\endgroup\$
1
\$\begingroup\$

Here's an idea that brings the concept of encapsulation into the mix:

class Customer
{
    public ushort Id { get; private set; }

    public Phone Phone { get; private set; }

    public Customer(ushort id, Phone phone)
    {
        Id = id;
        Phone = phone;
    }
}

class Provider : Customer
{
    public Provider(ushort id, Phone phone, ServiceField serviceField): base(id, phone)
    {
    }

    public ServiceField ServiceField { get; private set; }
}

class Person
{
    public string FirstName { get; private set; }
    public string LastName { get; private set; }
}

class Business
{
    public string Name { get; private set; }
    public Email Email { get; private set; }
}

class PrivateProvider : Provider
{
    public Person Person { get; private set; }

    public PrivateProvider(ushort id, Phone phone, Person person, ServiceField serviceField) : base(id, phone)
    {
        Person = person;
        ServiceField = serviceField;
    }
}


class BusinessProvider : Provider
{
    public Business Business { get; private set; }

    public BusinessProvider(ushort id, Phone phone, Business business, ServiceField serviceField) : base(id, phone)
    {
        Business = business;
        ServiceField = serviceField;
    }
}

class PrivateGeneric : Customer
{
    public Person Person { get; private set; }

    public PrivateGeneric(ushort id, Person person, Phone phone) : base(id, phone)
    {
        Person = person;
    }
}

class BusinessGeneric : Customer
{
    public Business Business { get; private set; }

    public BusinessGeneric(ushort id, Phone phone, Business business) : base(id, phone)
    {
        Business = business;
    }
}

And every time you come with a new classification like city vs. country, there are several options including:

  1. Make an enum and a new property of the enum type - i.e. enum Location
  2. Create a new class and encapsulate it - i.e. class Location like the example does with Person and Business.
  3. Create a new class in the inheritance tree - i.e. class CityProvider

Which option to choose depends largely on how the new classification impacts the logic.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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