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 still a relative novice with OOP, so I am still trying to understand some of the best practices for how to design "good" objects. I know this question is probably a rather subjective, which is why I am here rather than on stackoverflow.

I am designing an object that will have 20 separate string properties, none of which are required. These constraints are fixed and cannot be changed as it is an implementation of an outside data source that I have no control over.

The design problem I am running into is how to effectively construct an instance of the object and be able to compare equality by comparing each field without violating normal best practices.

  • A 20 argument constructor seems very wrong, especially since every field is a string, and there are cases where many of them will be null. I could use the optional arguments in C#4 to reduce the number of arguments that are actually called, but since everything is a string, I would have to use named parameters when calling the constructor, and that seems that it would be even uglier.

  • I have thought about trying to group the properties into smaller classes, but the most logical grouping still has an object with 10 properties, and at least half of the remaining properties have no logical grouping.

  • I could give each property a public setter, but that would make the object mutable, and thus complicate matters for overriding GetHashCode(). And I really don't need the object to be mutable. Once it is built, there should never be a reason to change it, but I don't want to rely on that because who knows what someone will try to do with it.

  • Implementing IEquatable<T> is a possibility for the equality test, but my understanding is it is recommended to also override Equals(object obj) and GetHashCode() when implementing IEquatable so all Equals methods have the same behavior.

Is this an edge case where overriding Equals without overriding GetHashCode should be done? I don't intend on using this object as a key in a dictionary or as a member of a HashSet, but I can't predict what someone else would try to do in the future.

Or should I just abandon the idea of doing an equality comparison on all of the properties with Equals and implement my own Compare() method?

Or is there a better method for building the object that would allow it to still be immutable without having to pass all of the arguments through a constructor or a method?

EDIT: In response to Sign's comment, the purpose of the object is ultimately to be able to compare the data from the outside data source to our internal data to ensure it is accurate. Both data sets contain address information, but they are broken up differently.

The internal data, is generally stored as a typical address (Street, City, State, Postal Code, County, Country), but there are variations when you have a suite/apartment # or a floor in a building.

The external data is broken down even further into smaller pieces.

So this object is supposed to be a common link between the 2 data sources and allow the user to compare like fields for equality.

EDIT #2: Ultimately, I chose Doc Brown's answer because a modified version of his dictionary idea (with Enums as Keys instead of strings) was the simplest way to go without having a messy constructor and still provide the immutability I was hoping to have. In the long term, I am going to experiment with some of Oliver's suggestions as some of his edits did have a lot to offer.

share|improve this question
    
I think this might be better suited for StackOverflow... –  Mr. Ant Dec 23 '11 at 14:08
    
@Ant I was on the fence about which site would be better, but i decided that there might not be a real "right" answer to this question, so i decided to come here instead –  psubsee2003 Dec 23 '11 at 14:11
    
@psubsee2003 What are you trying to do with this object? I have usually found that when I get to an object that is badly designed and I can't find a way to fix it, that I screwed up the design further up stream but it wasn't apparent until I got here. –  Sign Dec 23 '11 at 14:31
    
@Sign edited the question to clarify –  psubsee2003 Dec 23 '11 at 14:38

5 Answers 5

up vote 5 down vote accepted

If you insist of having your object to immutable, there is obviously no other way than providing the 20 strings through the constructor. And if you want to be able to leave some of the strings out, you must say which arguments you are providing and which are not, which leads you to some form of named parameters. Of course, besides the possibilities you mentioned by yourself, you can also

  • provide the 20 arguments by a list of strings
  • provide them by a dictionary (key=>value, where "key" is the attribute name)
  • provide them by an object of a helper class, which has the same 20 properties with getters and setters, so this one won't be immutable

It may also be a good idea to implement Equals and GetHashCodeby utilizing reflection, looping over all public string properties of your class.

share|improve this answer
    
An immutable object isn't a requirement, but in my limited experience, overriding GetHashCode properly requires the HashCode not change over the life of the object, and if it is mutable, I cannot guarantee that using standard methods of calculating a hashcode. –  psubsee2003 Dec 23 '11 at 14:53
    
And the dictionary as the argument in the constructor is a great idea, I hadn't even thought of that. +1 if i had enough rep to upvote. –  psubsee2003 Dec 23 '11 at 15:02
1  
@psubsee2003: beware, the dictionary in the constructor has a drawback. If you make a typo in one of the parameter names, the compiler won't notice. –  Doc Brown Dec 23 '11 at 16:34
    
This is the approach I've used in similar situations. Depending on the application I've used a Dictionary, a helper class or a List of helper class. –  jfrankcarr Dec 23 '11 at 17:33
1  
@DocBrown I had considered that drawback after I responded. I think the best solution to that problem is making the dictionary key an enum, rather than a string. –  psubsee2003 Dec 24 '11 at 10:26

Apparently, you are not interested in individual properties; you just want to compare a bunch of fields. Therefore, a dictionary or a list of fieldname/value pairs would seem very appropriate to me. It allows you to do the comparisons in a loop instead of having to treat each field individually. It is also a universal approach as you can apply it to different sets of properties. In addition, it is more flexible as you will not have to change the algorithms if you add new fields.

I would not stick to the immutable approach. Instead, give your class an Add-method and forget the constructor with the endless parameter list.

public class FieldComparer : IEquatable<FieldComparer>
{
    private Dictionary<string,string> _fieldDict = new Dictionary<string,string>();

    public void Add(string fieldName, string value) {
        fieldDict.Add(fieldName, value);
    }

    // The magic comes here
}

EDIT: As an answer to your concern, I have two suggestions for different approaches:


1

In the class described above, supplement individual properties for fields that you want to handle individually:

public string LastName {
    get {
        string last_Name;
        _fieldDict.TryGet("LastName", out lastName);
        return lastName;
    }
    set {
        _fieldDict["LastName"] = value;
    }
}

2

Use properties in the first place and make them available through an indexer in order to be able to loop through them:

public string FistName { get; set; }
public string LastName { get; set; }
public string City { get; set; }

public const int PropertyCount = 3;

public string this[int i]
{
    get
    {
        switch (i) {
            case 0: 
                return FirstName;
            case 1: 
                return LastName;
            case 2: 
                return City;
            default:
                throw new ArgumentNullException("Index out of bounds.");
        }
    }
    set
    {
        switch (i) {
            case 0: 
                FirstName = value;
                break;
            case 1: 
                LastName = value;
                break;
            case 2: 
                City = value;
                break;
            default:
                throw new ArgumentNullException("Index out of bounds.");
        }
    }
}

(The setter of the indexer might not be necessary for your purpose.) You can then access the properties like this:

var comp = new FieldComparer();
comp.FistName = "John";
Console.WriteLine(comp[0]); // -> John

EDIT: The solution #2 has the disadvantage, that we lose the universality of the approach. We could restore it, if we implemented the indexer and PropertyCount from an interface. (We also would have to convert PropertyCount to a read-only property). The Comparison would then happen in a separate class.

public interface IPropertyIndexer
{
    string this[int i] { get; }
    int PropertyCount { get; }
}

Instead, I will show a complete solution using an enumerator instead of an indexer. Both approaches are comparable. The enumerator version using yield return is a bit shorter than the indexer version using a switch statement.


3

public interface IPropertyEnumerator
{
    IEnumerator<string> AllValues { get; }
}

public class Address : IPropertyEnumerator
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string City { get; set; }

    public IEnumerator<string> AllValues
    {
        get
        {
            yield return FirstName;
            yield return LastName;
            yield return City;
        }
    }
}

public class Customer : IPropertyEnumerator
{
    public string Name { get; set; }
    public string CustNo { get; set; }

    public IEnumerator<string> AllValues
    {
        get
        {
            yield return Name;
            yield return CustNo;
        }
    }
}

public static class PropertyComparer
{
    public static bool Equals<T>(T x, T y)
        where T : IPropertyEnumerator
    {
        var enum1 = x.AllValues;
        var enum2 = y.AllValues;
        while (enum1.MoveNext()) {
            if (!enum2.MoveNext() || enum1.Current != enum2.Current) {
                return false;
            }
        }
        return !enum2.MoveNext(); // true if number of properties equal
    }
}

public class Example
{
    public void Compare()
    {
        var a1 = new Address { FirstName = "John", LastName = "Doe", City = "New York" };
        var a2 = new Address { FirstName = "John", LastName = "Doe", City = "New York" };
        Console.WriteLine(PropertyComparer.Equals(a1, a2));

        var c1 = new Customer { CustNo = "A123", Name = "Miller" };
        var c2 = new Customer { CustNo = "B777", Name = "Stanley" };
        Console.WriteLine(PropertyComparer.Equals(c1, c2));
    }
}
share|improve this answer
    
This is an intriguing idea, I have to give it some thought. It definitely could solve my current problem, but I have 2 concerns... 1) it limits the functionality of the class. It would only do a comparison and nothing else. A class with properties would be able to do add different unrelated methods in the future. 2) For my current solution, since my 2 data sources have slightly different formats, the add method would have to do some work to modify the value so the 2 formats would have the same dictionary Key-Value pairs. –  psubsee2003 Dec 24 '11 at 10:31

There is absolutely no problem with implementing GetHashCode() whether your object is immutable or not. If it is immutable, the hashcode can be precomputed; if it is mutable, the hashcode needs to be recalculated each time the GetHashCode() method is invoked. And since your object consists of nothing but 20 strings, which are immutable, and therefore their hashcodes are already computed, computing your hashcode is close to trivial. Just multiply the hashcodes of your non-null string members together, or search stackoverflow for some slightly better hashcode algorithm.

Or is there a better method for building the object that would allow it to still be immutable without having to pass all of the arguments through a constructor or a method?

I am a fanatic proponent of immutable objects, so I would advice you to make yours immutable. And I would not pass a dictionary or a list to the constructor; I would pass 20 separate arguments, there is nothing wrong with that, and there is nothing gained doing it any other way. Any other way is bound to move some of the checking that the compiler will do for you at compile time, to checking that you will have to do yourself at runtime: always a bad idea.

Is this an edge case where overriding Equals without overriding GetHashCode should be done? I don't intend on using this object as a key in a dictionary or as a member of a HashSet, but I can't predict what someone else would try to do in the future.

For IEquatable<T> it is true that you do not, strictly speaking, need to implement GetHashCode(), except to avoid the warning that you will receive if you don't, but good practice is to always implement GetHashCode(), precisely because someone might try to use it in the future.

Note: Since you will be making your object immutable, (won't you?) you can precompute your hashcode, and store it with the object, so that the Equals() method can first compare hashcodes and return immediately if they do not match, rather than always doing a full comparison. In practice you do not need to do tricks like that unless you have already established that you have a performance issue, but it is a trick worth keeping in mind.

Or should I just abandon the idea of doing an equality comparison on all of the properties with Equals and implement my own Compare() method?

I do not know what you mean by this question. You will be implementing your own Equals() method, and it will be a series of if( !Object.Equals( MyField1, other.MyField1 ) ) return false;

share|improve this answer
1  
If an object is not immutable, and you put it into a hash set, and the object content (and so the hash code) changes afterwards, you will most likely run into trouble. –  Doc Brown Dec 23 '11 at 16:37
    
Yes, of course. But the OP has explicitly stated "I don't intend on using this object as a key in a dictionary or as a member of a HashSet". And, to tell you the truth, this kind of object sounds like the kind of object that would be used as a value in a dictionary, not as a key. Regardless of that, the point I was trying to make is that the implementability of the GetHashCode method does not depend on whether the class is immutable or not. What depends on it is the suitability of using the class as a key. These are two different issues. –  Mike Nakis Dec 23 '11 at 16:50
    
The problem with 20 arguments is more human in my experience. I've implemented this before and gotten taken to task for it in code reviews. I'd also like to find out why you're a "fanatic proponent of immutable objects". Have you written anything about this somewhere? –  jfrankcarr Dec 23 '11 at 17:38
    
I have not written anything, but maybe it is time to do it, because I do have very specific reasons for it. When (and if) I do, I will post a comment here. –  Mike Nakis Dec 23 '11 at 17:49
1  
But in the mean time, you can read what Eric Lippert has written on the subject: blogs.msdn.com/b/ericlippert/archive/2007/11/13/… –  Mike Nakis Dec 23 '11 at 18:21

Depends on requirements of course... By efficient you mean execution speed, or dev speed / clarity?

Here is another possibility:

Have your constructor take a class object as it's only parameter. Certainly this "can" be easier to maintain and all the messy work to make it can be done elsewhere. This is definitely advisable where clarity is more important so that every field has a logical name and it's not meshed into a dictionary that may not get built properly. Who knows who will be working on your code next ;) Or maybe it will be you 2 years from now and you can save time and mistakes? But is that more important than execution speed? For me clarity trumps insignificant speed differences, up to a point where the accumulation is not insignificant of course ;)

share|improve this answer
    
By efficient, I was specifically referring to clarity. I don't want this to become a drag on performance, but I haven't even written any code yet, so performance hasn't entered my mind. I tend to subscribe to the idea that you shouldn't worry about performance until you know you have a problem. –  psubsee2003 Dec 24 '11 at 10:22

It seems like you want two constructors one for each type of input data. If that data is already contained in an object you could take that in. The Dictionary constructor doesn't seem like a good idea since an arbitrary set of data is not going to be sufficient to do anything with(an object with just a country and a street name would do no good).

share|improve this answer

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.