Skip to main content
added 61 characters in body
Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

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

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

Post Undeleted by Mathieu Guindon
Post Deleted by Mathieu Guindon
added 176 characters in body
Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463
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;
         }
    }
}
public class GSA : NMEAMsg
{
    private readonly IDictionary<Dop,float?> _tokens;

    public GSA(string[] 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;
         }
    }
}
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;
         }
    }
}
Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

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

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)
    {
        _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;
         }
    }
}