0
\$\begingroup\$

Can anyone help me with checking this code? It is for connecting to a database. The method getFromDatabase is to take values from the database and put it to a dataset, and the other method getFromDatabaseToCombo is to get from the database to a comboBox. I can't shake off this feeling that the last two methods can be combined somehow and something is not right in this class.

(the variables will be set to private later)

   public class dbConnect   
   {
    public MySqlConnection con;//declare variable for connection
    public MySqlCommand com;//varialbe for command
    public DataTable dt;
    public DataSet ds;
    public DataView myview;

    public dbConnect()  //Constructor
    {
        string serverName = "localhost"; //server name
        string dbName = "airline_db";//database name
        //string pass = "***"; //password
        string uid = "root";
        string conString = "server="+serverName+";User Id="+uid+";database="+dbName+"";

        con = new MySqlConnection(conString); //instance of con object

    }

    public void openConnect() //to open connection to database
    {
        if (con != null && con.State == ConnectionState.Closed)//to check if conn is already open or not
        {
            con.Open();
        }
    }

    public void closeConnect() //to close connection to database
    {
        if (con!=null && con.State == ConnectionState.Open)
        {
            con.Close();
        }
    }

    public void comText(string sqlQuery) //method to use sql 
    {
        com = new MySqlCommand(sqlQuery,con);
        //com.ExecuteNonQuery();
    }

    //to get from the database
    public DataTable getFromDatabase(string sqlQuery)
    {
        dt = new DataTable();
        ds = new DataSet();
        MySqlDataAdapter adp = new MySqlDataAdapter();

        comText(sqlQuery);
        adp.SelectCommand = com;
        adp.Fill(ds);
        dt = ds.Tables[0];      //all tables assignted as table[0]
        return dt;
    }
    public void getFromDatabaseToCombo(string sqlQuery) //check with above
    {
        dt = new DataTable();
        ds = new DataSet();
        MySqlDataAdapter adp = new MySqlDataAdapter(sqlQuery,"server=localhost;User Id=root;database=airline_db");
        adp.Fill(dt);
    }
}
}
\$\endgroup\$
1
\$\begingroup\$

Naming conventions:

You should follow the C# design guidelines and use PascalCase for public members, class names and methods. camelCase is for private members.

Give your members and methods meaningful names. Names like con or com don't mean anything, except for you. Try using sqlConnection and sqlCommand instead.

Method names should indicate what they do and/or return. So a method name like getFromDatabase is bad practice. What are you getting from the database? Instead, use GetUsers, GetRecords or GetData. Try to be specific, only use general names like the last example if there's no other way.

This also applies to the other methods, use Open or OpenConnection instead of openConnect. (same for Close)

String.Format:

Don't concatenate all sorts of variables with string constants. Use the String.Format method. Place the format and add your values as arguments. This results in cleaner code that is easier to read and maintain. Example, change this:

string conString = "server="+serverName+";User Id="+uid+";database="+dbName+"";

to:

var connectionString = String.Format("server={0};User Id={1};database={2}", server, userID, database);

Constructor + overloading:

Your constructor contains the values you use for the connectionstring. Don't place these values here as it makes the use of the constructor very limited. Create a separate method that will create the connection and call this method from the constructor. The default values you are using can be placed in a settings file or make them private constant fields:

private const string Server = "localhost";
private const string Database = "airline_db";
private const string UserID = "root";

Also, create another constructor that takes parameters so you can create a connection with other values. Combined you can overload the constructor and put all the logic in one. This looks clean and makes maintaining your code a lot easier. Example (classname is DatabaseConnector):

public DatabaseConnector() : this(Server, UserID, Database)
{
    //This will call the other constructor with the const values
}

public DatabaseConnector(string server, string userID, string database)
{
    mySqlConnection = CreateConnection(server, userID, database);
}

private MySqlConnection CreateConnection(string server, string userID, string database)
{
    var connectionString = String.Format("server={0};User Id={1};database={2}", server, userID, database);
    return new MySqlConnection(connectionString);
}

Methods:

comText is something I don't understand. It is a void method but doesn't do anything important enough to be a separate method. Let it return a MySqlCommand instead. Rewritten:

public MySqlCommand InitSqlCommand(string query) 
{
    var mySqlCommand = new MySqlCommand(query, mySqlConnection);
    return mySqlCommand;
}

Your DataTable and DataSet variables should be scoped inside the methods. There's no reason they should be global in the class. This way you can rewrite your getFromDatabase to following:

public DataTable GetData(string query)
{
    var dataTable = new DataTable();
    var dataSet = new DataSet();
    var dataAdapter = new MySqlDataAdapter { SelectCommand = InitSqlCommand(query) };

    dataAdapter.Fill(dataSet);
    dataTable = dataSet.Tables[0];
    return dataTable;
}

Things you'll also see changed is, the use of:

  • an object initializer for initializing members
  • the var keyword, this looks cleaner and you let the compiler do the job

Your Open and Close method should be private. The change in state should be determined by the class itself. Otherwise you can close the connection and try executing some command.

Dispose:

When using instances that handle unmanaged resources sucha as streams, connections, ... you have to dispose them. Otherwise they will not be claimed by the garbage collector. There are two ways to do this:

  1. Manually call the Dispose() method:

    private void Finalize()
    {
        if (mySqlConnection != null && mySqlConnection.State == ConnectionState.Closed)
            mySqlConnection.Dispose();
    }
    
  2. Use the using statement when using the commands.

More reading:

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for this helpful answer, ill rewrite the entire class \$\endgroup\$ – NimazSheik Nov 13 '14 at 13:11
  • \$\begingroup\$ Just another small doubt.You said the openConnection and closeConnection methods should be private so if i make it private i cant call it in any other class.Does this mean i shoould call in within the method (createConnection)? \$\endgroup\$ – NimazSheik Nov 13 '14 at 13:50
  • \$\begingroup\$ You can leave it public and call it from outside the class but I would make it private and call it from within the class wherever you need it. \$\endgroup\$ – Abbas Nov 13 '14 at 13:53
  • \$\begingroup\$ Okay thanks once again, you've helped me a lot! \$\endgroup\$ – NimazSheik Nov 13 '14 at 13:54
0
\$\begingroup\$

Idiomacy

Idiomatic c# expects PascalCase class names and public members. Member names should be a description of their function. Consider DbConnection, OpenConnection(), CloseConnection(), etc.

OOP

Dependency Inversion

Your constructor is hard-coding the values needed for your connectionString. If this is simply demo code, either hardcode the entire string or do it right and have the values passed in.

Scope

  • MySqlConnection con is assigned in the constructor but never used.
  • string conString is scoped to the constructor but needs to be available in the `getFromDatabaseToCombo(string) function as written.
  • DataTable dt and DataSet ds are scoped to the class but should be restricted within the functions in which they're used.
  • MySqlCommand com is scoped to the class, but should really be local to methods in which it is needed. The comText method should return a MySqlCommand and callers should use the returned value.

Encapsulation

openCon and closeCon create an unnecessary requirement for the consumer to manage the state of this objects connection. These methods should be made private and used internally by the object. Furthermore, database connections use unmanaged resources which need to be disposed of properly to prevent undefined behavior and memory leaks. I would reccomend creating a private CreateConnection method and return the MySqlConnection directly. It can be put into a using(disposable) {} block to ensure it is properly disposed when no longer needed.

void comText(string) should not be public. It changes the state of the object, but there is nothing which can be done externally to capitalize on the changed state.

Other

getFromDatabase(string) dt does not need to be initialized to a new DateSet(), as it is over written later when you dt = ds.Tables[0];

getFromDatabaseToCombo(string) does nothing useful as written. All of the objects created in this method are abandoned as it returns void. Ideally it should call getFromDatabase and then operate on the DataTable that is returned. This function really doesn't seem to fit with this class at all -- It would work better in a DataTableAdapter class or as an extension method for a ComboBox

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.