Is it a "pattern smell" to put getters like "FullName" or "FormattedPhoneNumber" in your model?

Question

I'm working on an ASP.NET MVC app, and I've been getting into the habit of putting what seem like helpful and convenient getters into my model/entity classes.

For example:

public class Member
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string PhoneNumber { get; set; }

    public string FullName
    {
        get { return FirstName + " " + LastName; }
    }

    public string FormattedPhoneNumber
    {
        get { return "(" + PhoneNumber.Substring(0, 3) + ") " + PhoneNumber.Substring(3, 3) + "-" + PhoneNumber.Substring(6); }
    }
}

I'm wondering people think about the FullName and FormattedPhoneNumber getters.

They make it very easy to create standardized data formats throughout the app, and they seem to save a lot of repeated code, but it could definitely be argued that data format is something that should be handled in mapping from model to view-model.

In fact, I was originally applying these data formats in my service layer where I do my mapping, but it was becoming a burden to constantly have to write formatters then apply them in many different places. E.g., I use "Full Name" in most views, and having to type something like model.FullName = MappingUtilities.GetFullName(entity.FirstName, entity.LastName); all over the place seemed a lot less elegant than just typing model.FullName = entity.FullName (or, if you use something like AutoMapper, potentially not typing anything at all).

So, where do you draw the line when it comes to data formatting. Is it "okay" to do data formatting in your model or is that a "pattern smell"?

Note: I definitely do not have any html in my model. I use html helpers for that. I'm strictly talking about formatting or combining data (and especially data that is frequently used).

Answer 1

In your example, I like the FullName getter (for all the reasons you have given) but I do not like the FormattedPhoneNumber getter. The reason is: it's probably not that easy (once you have international phone numbers etc.) and if you place the logic for formating phone numbers in a method of Member, chances are you will need to refactor (or copy-paste caugh) once you need a formated phone number for Institution, Vendor etc. too.

EDIT: IMO it would be better to have a PhoneNumber class with a Formatted getter.

Answer 2

The things you need to consider when writing code: Is it correct? Is it readable? Is it efficient? Is it maintainable? I would argue, as @btilly mentioned, that it's not maintainable due to culture-specific formatting, but the question seems to be more general that that.

Using accessors like these makes your code more readable, and, depending on how you use it, might make other parts of your code much cleaner. In my opinion, that doesn't smell at all. It would start to smell if you had Formatting accessors for any kind of string you might want to print (public string FirstLastName; public string FullName; public string FullNameWithMiddleInitial; public string PhoneNumberWithAreaCode; public string PhoneNumberWithoutAreaCode; public string PhoneNumberWithCountryCode;, etc)

Or, to put it another way, using a pattern doesn't automatically make your code have "pattern smell". You need to abuse it if you want to earn that attribute.

Answer 3

Breaks the single responsibility principle. Why not make a phone number class, etc...?

Answer 4

For your example, I don't see it as too big a deal to use specific formats. It's one or two and all parts of the application use the same format.

Where that decision would start to break down is when you have the same data going to several different places requiring different formats.

If that happened, I'd be tempted to pull the Member class back to:

public class Member
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string PhoneNumber { get; set; }  
}

And then do different adapters for each target. For example, suppose the information was required in CSV format:

public static class CSVMemberAdapter
{
    public static string ToCSV(this Member mbr)
    {
         return mbr.Id + "," + mbr.LastName + "," + mbr.FirstName, "," mbr.PhoneNumber;
    }
}

Always assuming that you've sanitized the data so that no commas etc. are in the strings.

The adapter doesn't have to be an extension method, but for this make-believe case it seems to fit.