5

I'm trying to ascertain whether the use of multiple references to the same property is code smell / an anti-pattern, based on the needs of the organisation.

As an example, consider:

abstract class Person {

    public string Title { get; set; }
    public string Forename { get; set; }
    public string MiddleNames { get; set; }
    public string Surname { get; set; }

    public string GivenName  { get { return Forename; } set { Forename = value; } }
    public string FamilyName { get { return Surname; }  set { Surname = value; } }
    public string FirstName  { get { return Forename; } set { Forename = value; } }
    public string LastName   { get { return Surname; }  set { Surname = value; } }

}

As you can see, I have several other properties, (kind of) aliasing or giving alternate names to properties in the object, but basically repeating exactly the functionality of the base property without exception.

The reasons behind this is down to the naming conventions utilised in different areas of the organisation; it's providing multiple ways of addressing the same information, allowing each different area to use their preferred method of access. Because each of the other properties reference the base property, any programmatic changes only need to be included in the said base property.

And so to clarify...

@GregBurghardt has kindly posted alternatives to my conundrums, but I feel that I must qualify one of my statements a little more.

In VB.NET I can quite happily code interfaces into my classes but use alternative names in the actual member implementation, like so...

Public Interface IPerson
    Property Forename() As String
    Property Surname() As String
End Interface

Public Class BillingPerson
    Implements IPerson

    Public Property GivenName() As String Implements IPerson.Forename
    ...
    Public Property FamilyName() As Int32 Implements IPerson.Surname
    ....
End Class

Notice, here, that I've used a different property name for the (VB) property, but still implemented the true name of the interface (e.g. Public Property GivenName () As String Implements IPerson.Forename). This means that I can reference my BillingPerson class, despite the obvious property name differences, using the interface...

'Define our interface type object here...
Dim myP As IPerson = New BillingPerson()
'Assign values directly to the interface members...
myP.Forename = "Fred"
myP.Surname = "Bloggs"
'Output the interface members...
Console.WriteLine(myP.Forename & " " & myP.Surname)

...or I can refer to the original class directly....

'Define our BillingPerson object here...
Dim myP As BillingPerson
'Assign values directly to the interface members...
myP.GivenName = "Fred"
myP.FamilyName = "Bloggs"
'Output the interface members...
Console.WriteLine(myP.Forename & " " & myP.Surname)

Using this methodology, my problem would be very short-lived , and I'd be able to create as many classes based on the interface as I need.

This doesn't appear to work the same way in C#, however, where I can't directly alias the name of the interface members.

  • 1
    I do think it's a code smell. If they insist in not having a single uniform iface, wouldn't an organization-specific view of Person, each with their org-specific terms, work (e.g. FamilyPersonView using FamilyName property)? It'd simply refer to Person.Name internally (i.e. Person instance is the model in the MVC-like design (the controller is the client)). – code_dredd May 24 '17 at 11:02
  • @ray: Well, it's not simply a case of unifying everyone (I wish it was!). Some of the properties used would be there to reflect organisational standards, some to reflect external system standards, and some to maintain old code. Yep - a complete mish-mash, sadly. Sadly, this is a very simple example. There are some more complicated data differences which cannot be aliased so easily, for instance, some of our systems use 'address1', 'address2', etc, whereas the organisation standard is 'house number/name', 'street', 'district' and so on. Mapping the first to the second is a nightmare. – Paul May 24 '17 at 11:36
  • 1
    I've just changed some code to handle Chinese names correctly. "FirstName" and "LastName" become really, really confusing when they don't mean what you think they mean. And I have two forenames. I don't have middle name. – gnasher729 Jun 3 '17 at 7:11
  • @gnasher729: Thankfully the scope for the applications I'm writing are pretty limited and don't have an application outside of our country. But you're absolutely right; different countries, religions and cultures use different formats for names. We did have some Ghurka names in one database. We had to change the format of the database as some of the family names were up to 100 characters long... – Paul Jun 3 '17 at 18:49
7

Since your organization can't seem to agree on naming things, it feels like you need to architect your application assuming there will be lots of things they don't agree on. It sounds like it will only get worse.

Maybe what you need instead is to adopt more of a facade or adapter pattern.

So, here is your "standard" person:

public class Person
{
    public string Title { get; set; }
    public string Forename { get; set; }
    public string MiddleNames { get; set; }
    public string Surname { get; set; }

    // Enterprise wide behavior goes here
}

And the Billing Department can have its own way:

public class BillingDepartmentPerson
{
    private readonly Person person;

    public string GivenName
    {
        get { return person.Forename; }
        set { person.Forename = value; }
    }

    public string FamilyName
    {
        get { return person.Surname; }
        set { person.Surname = value; }
    }

    public string FirstName
    {
        get { return person.Forename; }
        set { person.Forename = value; }
    }

    public string LastName
    {
        get { return person.Surname; }
        set { person.Surname = value; }
    }

    public BillingDepartmentPerson(Person person)
    {
        this.person = person;
    }

    // Billing department specific behavior goes here
}

And so can the Shipping Department:

public class ShippingDepartmentPerson
{
    private readonly Person person;

    public string Title
    {
        get { return person.Title; }
        set { person.Title = value; }
    }

    public string Forename
    {
        get { return person.Forename; }
        set { person.Forename = value; }
    }

    public string MiddleNames
    {
        get { return person.MiddleNames; }
        set { person.MiddleNames = value; }
    }

    public string Surname
    {
        get { return person.Surname; }
        set { person.Surname = value; }
    }

    public ShippingDepartmentPerson(Person person)
    {
        this.person = person;
    }

    // Shipping department specific behavior goes here
}

Yes, there is some repetition of code, but now you have a way of encapsulating the "organization area specific" functionality and separate it from the common, enterprise wide functionality in the Person class.

It also means you can't accidentally invoke operations specific to the billing department in the context of the shipping department. It gives you a nice Separation of Concerns so refactoring these different areas can be isolated, and the effects of any refactoring jobs can be limited in scope (see also the Open/Closed Principal).


Paul commented:

if this had been VB.NET, it wouldn't have been a problem, I could have used an interface and aliased all the member names. But it isn't!

... in VB.NET I can do something like: Public Function x() As Boolean Implements IY.a. I haven't seen anything in C# to allow this.

C# does:

public class MyItems : IEnumerable<int>
{
    public IEnumerator<int> GetEnumerator()
    {
        throw new NotImplementedException();
    }

    // Implement an interface method explicitly
    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
    {
        throw new NotImplementedException();
    }
}

VB.NET and C# compile down to the same exact MSIL code that gets executed in the same exact Common Language Runtime.

C# and VB.NET, as long as you compare Visual Studio and .NET framework versions, support the same features.

  • If this had been VB.NET, it wouldn't have been a problem, I could have used an interface and aliased all the member names. But it isn't! (But I am tempted to switch for this). – Paul May 24 '17 at 13:04
  • I did find this, also, which would fit the bill. – Paul May 24 '17 at 13:08
  • @Paul: C# support for interfaces is on par with VB.NET. – Greg Burghardt May 24 '17 at 13:10
  • Not entirely, in VB.NET I can do something like: Public Function x() As Boolean Implements IY.a. I haven't seen anything in C# to allow this. – Paul May 24 '17 at 13:12
  • @Paul: Updated my answer regarding your questions about C# interfaces. – Greg Burghardt May 24 '17 at 13:25
4

It is not so much a code smell as it is an operation smell. I think you should ask those "different areas of the organisation" to agree on a company wide naming convention before you proceed. You are trying to cater to their failure to communicate and so make it harder to ever straighten this legacy out.

Put a term on it, give them two weeks to get back to you. Which they probably won't. Then choose whatever names you like and declare those to be the company standard.

  • 1
    Oh if it were so simple! As per my comment to @ray, in my OP, this has come about because of the organisation that we are. To be fair, the overarching organisation has produced a standard dataset, which is really how things should move along (and really I've got it the wrong way round in the code in my post). As stated, however, we have so many pieces of legacy code, external systems (with their own standards) and unbendable local standards, that ... well, I wouldn't know where to start to standardise the local code (politics will play a big part, too; I'm only a grunt!) – Paul May 24 '17 at 11:43
  • 2
    Well, whatever relevant legacy code there is, it will have to hook up to your new piece anyway, right? So you offering synonyms will not save anyone work. It looks to me you can go with the overarching standard then. It will be the least confusing and error prone solution. Make sure to mention the alternative names in the intellisense documentation though. You grunt you. – Martin Maat May 24 '17 at 12:21
1

I think you should not have alias properties but pick one, perhaps the most common and document the aliases in a xml comment:

/// <summary>
/// Gets person's first name also known as Forename.
/// </summary>
public string FirstName { get; set; }

If everyone would use multiple properties for the same thing we would have arrays and lists with such properties as:

public int Count { get; }
public int NumberOfItems { get; }
public int Length { get; }

It's not only difficult to maintain but also the learning curve is steeper. You need to know all of them to know that they actually are identical. If someone would like to use an alias anyway, he can always write his own extension.

  • Once again, though, this does come down to the organisation, not the individual to decide, hence the original reason behind the post. Technically, it's not difficult to maintain the code I've written anyway; the only changes I would have to make to the code would be for the base properties (for instance, I may want to set a Dirty flag and raise an event if one of the properties change - this would only have to happen in a single place, not in every aliased property). – Paul May 27 '17 at 10:04
  • 1
    @Paul and if you implement the IPropertyChanged interface you have to raise serveral events. This is crazy. I find it's insane to create alias properties because some people like other names better and you want to satisfy them all. There is not as single reasonable point for this design, everything speaks against this idea. A compromise would be to decide which name to use and not the opposite, to implement everyone's wishes. You should discuss this and decide and pick one name but apparently no one wants to decide so you implement everything so everyone can avoid the responsibility. – t3chb0t May 27 '17 at 10:14
  • This isn't about people 'liking' something over something else. I work in a large organisation with multiple disciplines throughout. Standards are bound to be different from one place to the next. As for implementing the IPropertyChanged interface, that's a good idea - I must admit I didn't know about that one. – Paul May 27 '17 at 22:34
0

As an alternative, one could still have the single class approach with your names as well as single properties, but provide a generic accessor using reflection:

public static T GetPropertyValue<T>(object obj, string propName) 
{ 
    return (T)obj.GetType().GetProperty(propName).GetValue(obj, null); 
}

In front of that one will need a lookup configuration(xml, json, etc) so that items like:

FamilyName

get transformed to:

SurName

Then Surname is passed into the reflective method. So, the naming aliases are stored in a configuration as opposed to code or additional classes. If new aliases are added, just update the "alias config".

Either access the property directly or use the more convoluted approach of letting the caller try and access the property using their own name the code will do the look and pull the value using the reflective method.

This keeps your code base clean. Keep in mind reflection access versus directly access will be slower.

  • Hmm. I can see where you're going with this, but I suspect that we're starting to slow down the whole process. Contacting a JSON service becomes pretty expensive. Besides, I would like to keep it all as simple as possible. – Paul May 25 '17 at 8:04

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.