Using a KeyValuePair<> as Property vs. Separate Class vs. something else

Question

What is the proper way to show "Admin Tables" in my "Business Objects"? I have the following on my Address object.

public class Address
{
    public int AddressID { get; set; }
    public KeyValuePair<short, string> County { get; set; }
    ...
}

Now how would I instantiate this object, as far as the KeyValuePair<,> properties go?

My guess is:

var myAddress = new Address { AddressID = 3, County = new KeyValuePair<short, string>(32, "La Crosse")}

EDIT

This is what I am replacing with the KeyValuePair<> on the recommendations of another Programmer.

.....Address.cs..... 
public County County { get; set; }

.....County.cs.....
public class County
{
    public short? CountyID { get; set; }

    public string CountyName { get; set; }
}

Is there a better way between the two or a third way that is even better?

Are the values for the KVP coming from a database? Is it possible to extract them to an enumerated type so your magic numbers/characters are a bit less magical? — 48klocs, Feb 20 '12 at 17:04
I would advise against using a `KeyValuePair` in this situation; a developer cannot clearly see what the `Key` or `Value` actually is (what is the `short` `Key` for `County`?. Make your own type which has appropriate property names to make life easier for yourself and colleagues. — Lukazoid, Feb 20 '12 at 17:06
I have removed irrelevant members. Please revert if you disagree. — CodesInChaos, Feb 20 '12 at 17:08
@Lukazoid: Please see my edit and let me know if this makes more sense or if your statements still hold. — Refracted Paladin, Feb 20 '12 at 17:12
@Lukazoid or perhaps an enum, that is what the OP seems to be simulating with the KVP — Ben Robinson, Feb 20 '12 at 17:12
@CodeInChaos: So my guess at the end is correct, that is how I would instantiate an object that has a `KeyValuePair<>` property? — Refracted Paladin, Feb 20 '12 at 17:13
@RefractedPaladin That seems much better to me and should make the code a lot easier to understand for everyone. — Lukazoid, Feb 20 '12 at 17:14
@48klocs: I appologize but I would not know where to start with your suggestion as this is all pretty new to me. As a guess; I would populate some enumerated types from the DB and then use that in my app? Do this for all applicable Admin Tables I am guessing... — Refracted Paladin, Feb 20 '12 at 17:15
@BenRobinson An enum would be another solution, however enums are very static, whereas it seems the OP wants more dynamic data. — Lukazoid, Feb 20 '12 at 17:16
@Lukazoid: The way shown in my Edit you mean? The separate object for County. — Refracted Paladin, Feb 20 '12 at 17:16
@RefractedPaladin Yes, the edit is what I mean, the separate object for `County` is probably the approach I would have gone for. — Lukazoid, Feb 20 '12 at 17:17
Sorry all for the confusion. I guess my question really had a deeper question of KeyValuePair Property vs. Seperate Class for all my Admin types. I apologize for the confusing nature this Question has taken on... — Refracted Paladin, Feb 20 '12 at 17:18
@RefractedPaladin as Lukazoid said, the approach you take is going to depend on where the values live - if they can change dynamically then you'll want to build up a list of valid counties/county IDs. You'd want to convert that KVP to a type. If it's defined in code and is static as of compile time, an enumeration type is a far simpler way to accomplish the same goal. — 48klocs, Feb 20 '12 at 17:43

CodesInChaos · Answer 1 · 2012-02-20T17:29:43.280

3

I just ran your code, and it worked as expected.

The country property has correct value Key = 32 and Value = La Crosse.

Your new code is ugly. I'd either remove the setter of the Country property, or make the Country class immutable. This kind of double mutability, is a bug waiting to happen.

Making the Country class immutable, is probably the right decision, since the Id=>Name mapping is fixed.

I'd use:

public class County
{
    public short? ID { get; private set; }
    public string Name { get; private set; }

    private Country(short? id,string name)
    {
      ID=id;
      Name=name;
    }
}

edited Feb 20 '12 at 17:29

answered Feb 20 '12 at 17:10

CodesInChaos

106,488
23
218
262

Then if the name of a `County` needed to be change? Which is a very likely possibility. It doesn't feel right to be creating a new `County`, when what is desired it to actually update an existing one. I'm interested in hearing what your approach would be to this. – Lukazoid Feb 20 '12 at 17:24
Most of the time you don't need to change a country name at runtime, only once at load time. It's not like countries change there names all the time. But even if you do, the change should only happen from inside the Country class itself, and not through a place where a country is just used. For example by loading a new country definition file. – CodesInChaos Feb 20 '12 at 17:31

score 3 · Answer 2 · answered Feb 20 '12 at 17:12

KeyValuePair<T1, T2> buys you nothing in this case.

Why not just be explicit?

public class Address
{
    public int AddressID { get; set; }
    public int CountyCode { get; set; }
    public string CountyName { get; set; }
}

or another version would be that you define a type County with the two properties, then have a property of that type instead.

In code, clarity is king.

score 0 · Answer 3 · answered Feb 20 '12 at 17:12

0

Lukazoid gives a good hint why not to do this, bus in fact, the Initialization you are showing would work well. You could have proofen this rather easy using your Debugger. What is the question?

answered Feb 20 '12 at 17:12

eFloh

2,098
20
24

score 0 · Answer 4 · answered Feb 20 '12 at 17:15

0

Create a Country object so it is clear what that short and string are supposed to represent.

answered Feb 20 '12 at 17:15

Daniel N

419
8
20

Using a KeyValuePair<> as Property vs. Separate Class vs. something else

4 Answers4