New to OOP; is this a good class design?

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

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.

• Database Communication Object

• Data Transfer Object

• Business Logic Object

StackExchange Code Review Q#4394, answer score: 13