Properties and tryparse

Question

I have a class with 3 properties, and I would like to know how I can write my code better. I want to use Single.TryParse so that if any token is unknown string, I could still get 0. But using this method, I need to use a temp variable, and as my properties list increases, I will have more temp variable, and this is not efficient. Is there any way to code this better?

public class GSA : NMEAMsg
{
    public float Pdop { set; get; }
    public float Hdop { set; get; }
    public float Vdop { set; get; }

    protected override void parseTokens(string[] tokens)
    {
        float pdop = 0;
        float hdop = 0;
        float vdop = 0;

        Single.TryParse(tokens[tokens.Length - 3], out pdop);
        Single.TryParse(tokens[tokens.Length - 2], out hdop);
        Single.TryParse(tokens[tokens.Length - 1], out vdop);

        Pdop = pdop;
        Hdop = hdop;
        Vdop = vdop;
    }
}

Answer 1

I have a class with 3 properties, [...] and as my properties list increases, [...]

Wait, stop - here, that.

Classes are the blueprint for an object - the public members are that object's interface, and that shouldn't need to change that way. The Open/Closed Principle tells us that classes should be opened for extension, and closed for modification.

I don't know what any of these members mean, but a design that needs to change all the time is a design that didn't integrate the need to change as a requirement.

A design that embraces change, is one that minimizes the amount of code that's required to add to extend a piece of existing functionality.

If, instead of saying "I need a class with a P, H and V 'dop' float values, and maybe a X, Y and Z too, eventually", you said "I need a class with an unknown bunch of 'dop' float values; for this version I'll have P, H and V values, nobody knows what future holds"... the design would have reflected that.

void parseTokens(string[] tokens)

You have a string[] array here, an IEnumerable<string> ready to play with: use it to your advantage, and ask yourself how you could possibly structure things so that parseTokens (which should really be ParseTokens - conventions FTW!) can be implemented with an iterative logic.

Consider an enum:

public enum Dop // whatever that means
{
    Pdop,
    Hdop,
    Vdop
}

To add support for a new property, you add an enum member. Then you can have a function that returns this:

return tokens
    .Select((value, index) =>
        {
            float dop; 
            var success = float.TryParse(value, out dop); 
            return new 
            {
                Key = (Dop)index, 
                Success = success, 
                Value = dop
            };
        })
    .ToDictionary(item => item.Key, item => item.Success ? item.Value : (float?)null);

Notice float.TryParse is the same as Single.TryParse.

That's a Dictionary<Dop,float?>, where the float? value is null when the string couldn't be parsed into a float; you would take that and assign it to a private readonly field.

That's nice, but a Dictionary is not exactly the type of data structure you usually want to expose in your public interface - how about an indexer?

public float? this[Dop dop]
{
    get
    {
        float? value;
        return _tokens.TryGetValue(dop, out value)
            ? value
            : null;
     }
}

_{(assuming private readonly IDictionary<Dop,float?> _tokens; assigned in constructor)}

That said, I'd like to bring your attention to the indexing here:

Single.TryParse(tokens[tokens.Length - 3], out pdop);
Single.TryParse(tokens[tokens.Length - 2], out hdop);
Single.TryParse(tokens[tokens.Length - 1], out vdop);

Whenever tokens has less than the expected number of items, things blow up. What's the index of the pdop value in the tokens array? As a maintainer you shouldn't have to be bothered with these riddles.

What does using an enum get us? Imagine the client code has an instance of a GSA class, named gsa:

var pdop = gsa[Dop.Pdop];
var hdop = gsa[Dop.Hdop];
var vdop = gsa[Dop.Vdop];

If gsa has a float value for Pdop, then pdop will contain that value. Otherwise, it will be null.

And to extend it, you now only need to do this:

public enum Dop
{
    Pdop,
    Hdop,
    Vdop,
    Xdop,
    Ydop,
    Zdop
}

...and make sure the tokens array provided has as many arguments / fail fast:

protected override void ParseTokens(string[] tokens)
{
    if (tokens.Length != Enum.GetNames(typeof(Dop)).Length)
    {
        throw new ArgumentException("Invalid number of tokens.");
    }

    //
}

---

Another thing that strikes me as not needed and potentially bug-prone, is exposing the setters for these hard-earned float values.

Also, the order of the accessors is unusual:

public float Pdop { set; get; }

I'm used to see the getter first.

public float Pdop { get; private set; }

With a private setter, the only way to set that value is to parse a new array of strings.

At this point I'd question the mutability of the type. How about simply this?

public class GSA : NMEAMsg
{
    private readonly IDictionary<Dop,float?> _tokens;

    public GSA(string[] tokens)
    {
        if (tokens.Length != Enum.GetNames(typeof(Dop)).Length)
        {
            throw new ArgumentException("Invalid number of tokens.");
        }

        _tokens = tokens
            .Select((value, index) =>
                {
                    float dop; 
                    var success = float.TryParse(value, out dop); 
                    return new 
                    {
                        Key = (Dop)index, 
                        Success = success, 
                        Value = dop
                    };
                })
            .ToDictionary(item => item.Key, item => item.Success ? item.Value : (float?)null);
    }

    public float? this[Dop dop]
    {
        get
        {
            float? value;
            return _tokens.TryGetValue(dop, out value)
                ? value
                : null;
         }
    }
}