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