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 value with constant pieces of string. 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:
Manually call the Dispose() method:
private void Finalize()
{
if (mySqlConnection != null && mySqlConnection.State == ConnectionState.Closed)
mySqlConnection.Dispose();
}
Use the using statement when using the commands.
More reading:
