What’s Wrong With This Code: Type Design

Here is something I see all the time. Maybe done by lazy programmers. This is real in production code. What is wrong with this type design and why?

public class Metadata
{
    public int videoId = 0;
    public string Filename = string.Empty;
    public long Filesize = 0;
    public double Speed = 0;
    public long Time = 0;
    public double Altitude = 0;
    public double Longitude = 0;
    public double Latitude = 0;
    public double Accuracy = 0;
    public double Bearing = 0;
    public bool HasBearing = false;
    public bool HasSpeed = false;
    public bool HasAltitude/* = false*/;
    public bool HasAccuracy/* = false*/;
    public string deviceId = string.Empty;
    public string dateReceived = string.Empty;
    public string dateTransmitted = string.Empty;

    //Rest of code removed for brevity
}

Submit your answers by using comments below. I see at least three issues, one being major.

 


Discover more from dotNetTips.com

Subscribe to get the latest posts sent to your email.

2 thoughts on “What’s Wrong With This Code: Type Design

  1. 1. All are public. Should be private and implement properties. Then the flexibility of readonly properties would be possible.
    2. Several are initializied with the type defaults, which results in double work.
    3. Variable names are not consistent or follow any standard.

  2. The class name is not all that descriptive, it leaves out some context. It appears to be metadata for a file, and perhaps the files in question are in-flight or in-movement recordings. I’d change the name to something more specific. There appear to be two kinds of data here: data about the file and data about what was going on when the file was created. Hopefully putting them together in the same class makes sense in context of the overall system.

    I’d rather see all these data members be private with public properties.

    HasAltitude and HasAccuracy defaults are commented out. Bad practice.

    That said, all the Has* members should be readonly properties based on the values of their corresponding data members.

    date* fields ought to actually be a date type, not strings. Same sort of nit for Time.

    Altitude/Longitude/Latitude/Accuracy/Bearing are initialized to zeroes, each of which is a valid value. Better to make the doubles nullable or default to values for each that are not valid, changing the Has* properties accordingly.

    Casing of names is inconsistent.

Leave a comment

This site uses Akismet to reduce spam. Learn how your comment data is processed.