New to OOP; is this a good class design?

Question

I have a few different classes in my application, but this specific class is the one I'm interested about. Please review for good practices and potential problems.

using ExactEarth.Utilities.Jaguar.Properties;
using System;
using System.Data;
using System.Data.SqlClient;
using System.IO;

public class Downlink
{
    public string PassId { get; set; }
    public string DataId { get; set; }
    public string NoradId { get; set; }
    public string Spacecraft { get; set; }
    public string GroundStation { get; set; }
    public string Start { get; set; }
    public string End { get; set; }
    public string Status { get; set; }

    private static string connectionString;
    private static SqlConnection connection;

    public Downlink(string passId)
    {
        using (StreamReader reader = new StreamReader(AppDomain.CurrentDomain.BaseDirectory + Resources.connectionStringPath))
        {
            connectionString = reader.ReadLine();
        }

        using (connection = new SqlConnection(connectionString))
        {
            connection.Open();
            this.PassId = passId;
            this.DataId = GetDataId();
            this.NoradId = GetSatelliteId();
            this.Spacecraft = GetSpacecraft();
            this.GroundStation = GetGroundStation();
            this.Start = GetStartTime();
            this.End = GetEndTime();
            this.Status = GetPassStatus();
        }
    }

    public string GetDataId()
    {
        string dataId = null;

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@passId", SqlDbType.NVarChar).Value = this.PassId;
            command.CommandText = Resources.passDataQuery;
            command.Connection = connection;
            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            dataId = reader["gs_pass_data_id"].ToString();
            reader.Close();
        }

        return dataId;
    }

    public string GetSatelliteId()
    {
        string satelliteId = null;

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
            command.CommandText = Resources.ausInPassQuery;
            command.Connection = connection;
            SqlDataReader reader = command.ExecuteReader();
            if (reader.HasRows)
            {
                reader.Read();
                satelliteId = reader["acq_id"].ToString().Substring(0, 5);
            }

            return satelliteId;
        }
    }

    public string GetSpacecraft()
    {
        string path = AppDomain.CurrentDomain.BaseDirectory;
        path += Resources.spacecraftPath;

        using (StreamReader reader = new StreamReader(path))
        {
            string input = null;
            while ((input = reader.ReadLine()) != null)
            {
                string satelliteId = input.Split(',')[0];

                if (this.NoradId == satelliteId)
                    return input.Split(',')[1];
            }
        }

        // Spacecraft not known
        return Resources.unknownSpacecraft;
    }

    public string GetGroundStation()
    {
        string station = null;

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
            command.CommandText = Resources.groundStationQuery;
            command.Connection = connection;
            SqlDataReader reader = command.ExecuteReader();
            if (reader.HasRows)
            {
                reader.Read();
                station = reader["ground_station_name"].ToString();
            }

            return station;
        }
    }

    public string GetStartTime()
    {
        string start = null;

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
            command.CommandText = Resources.passStartQuery;
            command.Connection = connection;
            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            start = Convert.ToDateTime(reader["downlink_start_time"]).ToString("yyyy-MM-dd HH':'mm':'ss");
            reader.Close();
        }

        return start;
    }

    public string GetEndTime()
    {
        string end = null;

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
            command.CommandText = Resources.passEndQuery;
            command.Connection = connection;
            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            end = Convert.ToDateTime(reader["downlink_end_time"]).ToString("yyyy-MM-dd HH':'mm':'ss");
            reader.Close();
        }

        return end;
    }

    public string GetPassStatus()
    {
        string status = null;

        if (!JaguarUtilities.IsSatelliteTracked(this.Spacecraft))
            status = Resources.statusNotTracked;

        else
        {
            using (SqlCommand command = new SqlCommand())
            {
                command.Parameters.Add("@passId", SqlDbType.NVarChar).Value = this.PassId;
                command.CommandText = Resources.sourceFileQuery;
                command.Connection = connection;
                SqlDataReader reader = command.ExecuteReader();

                if (reader.HasRows)
                {
                    reader.Close();

                    using (SqlCommand newCommand = new SqlCommand())
                    {
                        newCommand.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
                        newCommand.CommandText = Resources.passStatusQuery;
                        newCommand.Connection = connection;
                        reader = newCommand.ExecuteReader();
                        status = Resources.statusProcessing;
                        if (reader.HasRows)
                            status = Resources.statusProcessed;
                    }
                }

                else
                {
                    status = Resources.statusScheduled;

                    if (Convert.ToDateTime(this.End) < DateTime.UtcNow)
                    {
                        TimeSpan span = DateTime.UtcNow.Subtract(Convert.ToDateTime(this.End));
                        TimeSpan failTime = JaguarUtilities.GetFailTime(this.Spacecraft, this.GroundStation);

                        if (TimeSpan.Compare(span, failTime) <= 0)
                            status = Resources.statusUpdate + (Convert.ToDateTime(this.End) + failTime).ToString("HH':'mm");

                        else
                            status = Resources.statusWaiting;
                    }
                }
            }
        }

        return status;
    }

    public bool IsEscalated()
    {
        bool escalated;

        using (connection)
        {
            if (IsLastSpacePassFail())
                escalated = true;

            else if (IsNextSpacePassFail())
                escalated = true;

            else if (IsLastGroundPassFail())
                escalated = true;

            else if (IsNextGroundPassFail())
                escalated = true;

            else
                escalated = false;
        }

        return escalated;
    }

    private bool IsProcessingTooLong()
    {
        string path = AppDomain.CurrentDomain.BaseDirectory + Resources.processingTimePath;
        using (StreamReader reader = new StreamReader(path))
        {
            string time = reader.ReadLine();
            TimeSpan span = DateTime.UtcNow.Subtract(Convert.ToDateTime(this.End));
            TimeSpan failTime = JaguarUtilities.GetFailTime(this.Spacecraft, this.GroundStation);
            TimeSpan fail = new TimeSpan(0, int.Parse(time), 0);

            if (span > (fail + failTime))
                return true;

            else
                return false;
        }
    }

    private bool IsLastSpacePassFail()
    {
        string passId = null;
        connection.ConnectionString = connectionString;
        connection.Open();

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@satelliteId", SqlDbType.NVarChar).Value = this.NoradId;
            command.Parameters.Add("@downlinkTime", SqlDbType.NVarChar).Value = this.Start;
            command.CommandText = Resources.lastSpaceDataQuery;
            command.Connection = connection;

            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            passId = reader["gs_pass_id"].ToString();
        }

        Downlink lastDownlink = new Downlink(passId);
        if (lastDownlink.Status == Resources.statusWaiting)
            return true;

        else if (lastDownlink.Status == Resources.statusProcessing)
        {
            if (lastDownlink.IsProcessingTooLong())
                return true;
            else
                return false;
        }

        else
            return false;
    }

    private bool IsNextSpacePassFail()
    {
        string passId = null;
        connection.ConnectionString = connectionString;
        connection.Open();

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@satelliteId", SqlDbType.NVarChar).Value = this.NoradId;
            command.Parameters.Add("@downlinkTime", SqlDbType.NVarChar).Value = this.End;
            command.CommandText = Resources.nextSpaceDataQuery;
            command.Connection = connection;

            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            passId = reader["gs_pass_id"].ToString();
        }

        Downlink lastDownlink = new Downlink(passId);
        if (lastDownlink.Status == Resources.statusWaiting)
            return true;

        else if (lastDownlink.Status == Resources.statusProcessing)
        {
            if (lastDownlink.IsProcessingTooLong())
                return true;
            else
                return false;
        }

        else
            return false;
    }

    private bool IsLastGroundPassFail()
    {
        string passId = null;
        connection.ConnectionString = connectionString;
        connection.Open();

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@groundId", SqlDbType.NVarChar).Value = this.GroundStation;
            command.Parameters.Add("@downlinkTime", SqlDbType.NVarChar).Value = this.Start;
            command.CommandText = Resources.lastGroundDataQuery;
            command.Connection = connection;

            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            passId = reader["gs_pass_id"].ToString();
        }

        Downlink lastDownlink = new Downlink(passId);
        if (lastDownlink.Status == Resources.statusWaiting)
            return true;

        else if (lastDownlink.Status == Resources.statusProcessing)
        {
            if (lastDownlink.IsProcessingTooLong())
                return true;
            else
                return false;
        }

        else
            return false;
    }

    private bool IsNextGroundPassFail()
    {
        string passId = null;
        connection.ConnectionString = connectionString;
        connection.Open();

        using (SqlCommand command = new SqlCommand())
        {
            command.Parameters.Add("@groundId", SqlDbType.NVarChar).Value = this.GroundStation;
            command.Parameters.Add("@downlinkTime", SqlDbType.NVarChar).Value = this.End;
            command.CommandText = Resources.nextGroundDataQuery;
            command.Connection = connection;

            SqlDataReader reader = command.ExecuteReader();
            reader.Read();
            passId = reader["gs_pass_id"].ToString();
        }

        Downlink lastDownlink = new Downlink(passId);
        if (lastDownlink.Status == Resources.statusWaiting)
            return true;

        else if (lastDownlink.Status == Resources.statusProcessing)
        {
            if (lastDownlink.IsProcessingTooLong())
                return true;
            else
                return false;
        }

        else
            return false;
    }

    public int GetNumberAusInPass(string query)
    {
        int planned = 0;
        using (connection)
        {
            connection.ConnectionString = connectionString;
            connection.Open();

            using (SqlCommand command = new SqlCommand())
            {
                command.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
                command.CommandText = query;
                command.Connection = connection;
                SqlDataReader reader = command.ExecuteReader();
                while (reader.Read())
                {
                    ++planned;
                }

                reader.Close();
            }

            return planned;
        }
    }

    public int GetNumberAusMatched()
    {
        int matched = 0;

        using (connection)
        {
            connection.ConnectionString = connectionString;
            connection.Open();

            using (SqlCommand command = new SqlCommand())
            {
                command.Parameters.Add("@dataId", SqlDbType.NVarChar).Value = this.DataId;
                command.CommandText = Resources.producedAusQuery;
                command.Connection = connection;
                SqlDataReader reader = command.ExecuteReader();
                while (reader.Read())
                {
                    long num;
                    if (long.TryParse(reader["acquisition_id"].ToString().Split('_')[1], out num))
                        ++matched;
                }

                reader.Close();
            }

            return matched;
        }
    }
}

Answer 1

First off, might I suggest some reading material? SOLID OOP Design Principles

Now, onto your code. Two things jump out at me. Your class is easy to misuse, and you're attempting to do too many things with a single class.

1 Easy to misuse

You expose a series of Public properties that you populate from the constructor. Additionally, you expose the actual methods used to populate these fields from the database.

Why does this matter? It means that I could construct your class, query the DataId property, then call GetDataId and get different values.

Additionally, setting the various properties has no effect. At the very least you should declare these properties with private set functions.

public string DataId { get; private set; }

2 You've given this class too many responsibilities.

It appears to me that you want to do two things with this class. Access values from a database and pass them around to other parts of your application.

Split this into two classes. Write one class that knows how to connect and retrieve values from the database and another class whose sole job is to move these values around. This second class I'm describing is commonly referred to as a Data Transfer Object, or DTO.

Your database class would create and populate DTO's upon request for you. Moving to this pattern solves problem one for you too.

You also appear to have some business logic built into this class. Functions like IsNextGroundPassFail seem to be making decisions based on data found in the database. Decisions based on data and should be in a separate class from the code to read and write the database.

Summary:

I think you need at least three classes.

1. Database Communication Object
2. Data Transfer Object
3. Business Logic Object

Answer 2

No, its not....

firstly, all the database code is mixed in. This is better off being done using something like nhibernate / entityframework or some similar thing. ( lots of differing opinions on tech to use there! )

but you'd endup with something like

DataLink link = Repository.GetDatalinkByPassId("mypassid");

which would go away and create the datalink object from the database and populate all the fields, and if applicable, load other objects that the datalink references.

Next there's a whole lot of concepts satellites / ground stations / etc that are represented as strings. There may be objects here that you want to extract.

But you might also want to abstract things a bit, that its not between a ground station and a sat, but between Nodes? or perhaps some other abstract concept. I don't know enough about the domain to suggest actual abstractions, but the key in any OO system is finding abstractions.

If you have time, put it into a DateTime object, not a string

My suspicion is you are using this class mainly for display. This would be better handled through a "ViewModel" A view model would take things like DataLink object and convert things, like time, into a form that's for display ( it would be in charge of what format string to use to convert time to something displayable for instance, and could handle different time formats depending on whether you like time the most sensible way d m y, or the silly way m d y ;-) )

Answer 3

Agree with the previous comments regarding splitting of the class and confusing interface.

Furthermore, I'd suggest you to start using Test Driven Development, it makes it easier to, for example:

• figure out what should be public vs what should be private (if other classes never use the "read/get" methods and need only the properties, why expose implementation details),
• what should be static (ideally, nothing, otherwise you loose the ease of testing things in isolation, and ease of extension later in development),
• division of responsibilities (database access vs. data objects vs. business logic).