Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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).

share|improve this question
1  
Doing this can be convenient. But hope and pray that you never have to internationalize that code. –  btilly Jun 23 '11 at 21:40
    
@btilly, good point, but I'm about 99.99% confident that I won't. –  devuxer Jun 23 '11 at 21:45
    
Specific to FullName and PhoneNumber, definitely. Is the question specifically about those because they have inconsistent formats in different cultures, or did @DanM simply pick examples that don't internationalize well for a more general question? –  Greg Jackson Jun 23 '11 at 21:47
    
@Greg Jackson, definitely the ladder. As ammoQ pointed out, PhoneNumber probably belongs in its own class (which I've now implemented). But FullName was really the one that motivated me to write the question. But I'm interested in finding out whether, in general, it makes sense to put data formatting/combing, etc. in the model for things that will apply app-wide. From the answers below, it seems like this is not an anti-pattern, but the decision should be made carefully. –  devuxer Jun 24 '11 at 17:42
    
Keep in mind that this specific formatting for full name may also not internationalize well. In Eastern Asia, for example, the name order is reverse what it is in the Western world. Do you really need to explicitly handle this? Maybe not, but just keep in mind that there are a lot of tricky things you will come across when formatting data. –  Greg Jackson Jun 24 '11 at 17:46

4 Answers 4

up vote 9 down vote accepted

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.

share|improve this answer
    
+1 and thanks. But what if I actually put my phone number formatting code in an extension method? Then any model could use it, and there would be no refactoring or copy/pasting. This solution would still be a great deal less repetitive than applying the formatter to every phone number that appears in every view. –  devuxer Jun 23 '11 at 21:55
2  
Using an extension method for that would be a quick way to the "functional decomposition" antipattern. By using an extension method (for the String class, I assume), you "teach" the String class how to format phone numbers. Is it really the responsibility of the String class to know about phone numbers? I don't think so. Used like that, extension methods are syntactical sugar to let something that clearly isn't object oriented look like it was. –  user281377 Jun 23 '11 at 22:08
    
Okay, forget I said extension method. Pretend I said utility method or formatter class. I'm simply saying that refactoring/copy pasting shouldn't be necessary, regardless of whether I have a model getter or do the formatting somewhere else. –  devuxer Jun 23 '11 at 22:21
1  
And I do like the idea of a PhoneNumber class. I've been planning to do that anyway because I also have a PhoneType property. –  devuxer Jun 23 '11 at 22:23

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.

share|improve this answer
    
Thanks, Greg. +1. I agree with you about putting every combination. I'm really just trying to come up with the cleanest way to standardize how data is viewed. –  devuxer Jun 23 '11 at 22:01

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

share|improve this answer
    
Okay, I actually agree with that (see discussion under ammoQ's answer), but should I also make a FullName class? –  devuxer Jun 23 '11 at 23:08
    
Yes, you should, but you should correct the spelling from "FullName" to "FoolName". Or maybe "PersonalName", since it is the name of a person, and not a full. –  kevin cline Jun 23 '11 at 23:50
1  
Really? Unless the given class is expected to grow, just what exactly is wrong with it? Even if it does grow, how hard would it be to re-factor then? –  Job Jun 24 '11 at 3:02
    
@DanM: Then that's what you ask for, i.e. ask them to type in their full legal name. If you're sorting by first name, you're really sorting on the first letter (then the second, and so on) of the first name (then the next, then so on), so the names will sort the same regardless. –  Matt Ellen Jun 24 '11 at 18:53
    
@DanM let us continue this discussion in chat –  Matt Ellen Jun 24 '11 at 18:54

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.

share|improve this answer
    
It's been a while since I've written C#, but surely there are serialization classes that can handle this, instead of tediously writing methods like toCSV over and over and over and over and over and over and over and over and over and over and over and over and over and over and over and over ... –  kevin cline Jun 23 '11 at 23:48
    
@kevin cline: It depends on what you need at each sink. If all they need is serialized XML, fine. Many systems don't. If it has to be done over as many times as that, then there's something wrong with the design. –  Peter K. Jun 24 '11 at 0:47
    
typically there would be many classes to serialize. It should be possible to write a single CSV or other serializer that could handle most classes via reflection, rather than hand-coding them like your example. –  kevin cline Jun 24 '11 at 4:17
    
@kevin cline: Violently agree! I was just writing something very specific for the question asked. Concrete, simple examples tend to explain things better. –  Peter K. Jun 24 '11 at 11:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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