# Database Connection Program

Earlier today I asked a question as a guest (whoops) regarding my Database program. I could not comment nor post again asking for further advice from the people that answered my question. I have worked on the advice given to me in that answer which can be seen here; Database Connection Program With HR Functionality

I am still looking for advice on my further classes, something I cannot ask for in the previous question. Being completely new to CodeReview, I shall post again in the hope I can still find advice on my code, however if this is the wrong thing to do then please do notify me.

User Selection

public partial class UserSelection : Page
{
public UserSelection()
{
InitializeComponent();
}

/***
*
*  Take all the data from the DataAccessor method FillDataGrid() and Trim() white space
*
***/

private void FillDataGrid(object sender, RoutedEventArgs e)
{
DataAccessor da = new DataAccessor();
DataTable dt = da.FillDataGrid();

foreach (DataRow dr in dt.Rows)
{
if (!dr.IsNull("em_netname"))
{
dr["em_netname"] = dr["em_netname"].ToString().Trim();
}
if (!dr.IsNull("em_dept"))
{
dr["em_dept"] = dr["em_dept"].ToString().Trim();
}
if (!dr.IsNull("em_name"))
{
dr["em_name"] = dr["em_name"].ToString().Trim();
}
if (!dr.IsNull("em_init"))
{
dr["em_init"] = dr["em_init"].ToString().Trim();
}
}
dataGrid.ItemsSource = dt.AsDataView();
}

/***
*
*  Load the user's image from the S drive, if no image exists load noimage.png
*
***/

private void LoadUserImage(object sender, SelectionChangedEventArgs e)
{
try
{
DataRowView dataRow = (DataRowView)dataGrid.SelectedItem;
userImage.Source = new BitmapImage(new Uri(@"S:\Picture\"+username+".jpg"));                                            //"
}
catch
{
userImage.Source = new BitmapImage(new Uri(@"C:\Users\DanD\Desktop\noimage.png"));
}
}

/***
*
*  On clicking the HR button, load the HRsystem with the desired person
*
***/

private void hrButton_Click(object sender, RoutedEventArgs e)
{
DataAccessor da = new DataAccessor();
DataRowView dataRow = (DataRowView)dataGrid.SelectedItem;

if (dataRow != null)
{
Properties.Settings.Default.UserID = dataRow.Row.ItemArray[0].ToString(); // Add the selected Users ID to the properties settings file
Properties.Settings.Default.Save();
da.SetUserDetails();
} else
{
}
}

/***
*
*  Chane the names of the existing columns, called when the columns are generated
*
***/

private void ChangeColumnNames(object sender, DataGridAutoGeneratingColumnEventArgs e)
{
if (e.PropertyName.StartsWith("em_pplid"))
{
}
if (e.PropertyName.StartsWith("em_name"))
{
}
if (e.PropertyName.StartsWith("em_netname"))
{
}
if (e.PropertyName.StartsWith("em_init"))
{
}
if (e.PropertyName.StartsWith("em_dept"))
{
}
}

/***
*
*  Search the grid based on which radio button is selected, called when text is entered into the text box
*
***/

private void SearchGrid(object sender, TextChangedEventArgs e)
{
DataView dv = dataGrid.ItemsSource as DataView;
{
dv.RowFilter = "em_netname LIKE '%" +searchBox.Text+ "%'";
}
{
dv.RowFilter = "em_dept LIKE '%" + searchBox.Text + "%'";
}
{
dv.RowFilter = "em_name LIKE '%" + searchBox.Text + "%'";
}
{
dv.RowFilter = "em_init LIKE '%" + searchBox.Text + "%'";
}
}

private void AddEmployee(object sender, RoutedEventArgs e)
{
}
}


Data Accessor

class DataAccessor
{

/***
*
*  Select all of the valid users for use by the DataGrid in UserSelection.xaml
*
***/

public DataTable FillDataGrid()
{
string constr = ConfigurationManager.ConnectionStrings["dbfString"].ConnectionString;
using (OleDbConnection dbfCon = new OleDbConnection(constr))
{
try
{
dbfCon.Open();
DataTable dTable = new DataTable();
OleDbCommand MyQuery = new OleDbCommand("SELECT em_pplid, em_name, em_netname, em_init, em_dept FROM employs WHERE em_netname NOT LIKE '' AND em_type != 2", dbfCon);
return dTable;
}
catch (OleDbException)
{
throw;
}
}
}

/***
*
*  Select the user's details and place into a list<String> for use by the HRsystem
*
***/

public List<string> SetUserDetails()
{
var userID = Properties.Settings.Default.UserID;
string constr = ConfigurationManager.ConnectionStrings["dbfString"].ConnectionString;
using (OleDbConnection dbfCon = new OleDbConnection(constr))
{
try
{
dbfCon.Open();
var cmdString = string.Format("SELECT em_surname, em_name, em_netname, em_init, em_dept, em_title FROM employs WHERE em_pplid = {0};", userID);
OleDbCommand dbfCmd = new OleDbCommand(cmdString, dbfCon);
List<string> listUser = new List<string>();
{
return listUser;
}
}
catch (OleDbException)
{
throw;
}
}
return null;
}
}

• Welcome to Code Review! This is indeed the correct way to get feedback on an updated script. Also your original question wasn't posted as a guest, but as the account CBR is that perhaps the username for another Stack Exchange account you have? – SuperBiasedMan Oct 12 '15 at 13:19
• @SuperBiasedMan I've actually just noticed, I have no idea how I have managed to do that! That is indeed me and must be my account, however I use this account across stack overflow. Weird I'll have to look into it more but will stick to using this account for now. – CBR Oct 12 '15 at 13:22
• @CBreeze You can ask for both accounts to be merged: codereview.stackexchange.com/help/merging-accounts – BCdotWEB Oct 12 '15 at 13:52
• @BCdotWEB Fantastic I have merged accounts. – CBR Oct 12 '15 at 14:02

Before I get started, I would suggest using an ORM for database access whenever possible. However, the remainder of this answer will assume that is not an option (for whatever reason).

## Use the MySQL ADO connector library

You are currently using OleDB to access your MySql database (indicated by tag), but there is actually a MySql-specific library you can use. It uses the ADO.NET interfaces, so your code will have few changes aside from which objects you work with (e.g., MySqlConnection instead of OleDbConnection).

It appears that the MySql team also maintains a MySql.Data nuget package as well, making this even easier:

Install-Package MySql.Data


## Use parameterized queries instead of string formats

var cmdString = string.Format("SELECT em_surname, em_name, em_netname, em_init, em_dept, em_title FROM employs WHERE em_pplid = {0};", userID);


Building queries with string.Format is dangerous, as it leaves you open to SQL Injection attacks. This particular case is using a value from the app settings, so perhaps it's not likely in this case, but it's a good idea to get into the habit of using parameterized queries anyways.

For MySql, you inject @-delimeted names: var cmdString = string.Format("SELECT em_surname, em_name, em_netname, em_init, em_dept, em_title FROM employs WHERE em_pplid = @userID");

dbfCmd.Parameters.Add(new MySqlParameter("@userID", userID));


## Dispose all the things

While you have a using statement for your connection object, there are a few other objects which implement IDisposable you missed. Both the command and reader objects need using statements.

## Naming

The SetUserDetails method doesn't set anything. It would be more appropriate to call it GetUserDetails, since you are retrieving information.

Your variable names could use some work as well - they are somewhat inconsistent. Some have prefixes, while others do not. Some are camelCase, while others are PascalCase (the convention is camelCase). I left these changes as an exercise for the reader :)

## Use well-defined data objects

I also noticed that SetUserDetails returns a List<string> which contains various user detail properties. It would be better to create a data structure to hold this and return it instead:

public class UserDetails
{
public string Surname { get; set; }
public string Name { get; set; }
public string Netname { get; set; }
public string Initials { get; set; }
public string Department { get; set; }
public string JobTitle { get; set; }
}


Then, your while loop can look as follows:

while(myReader.Read())
{
var details = new UserDetails
{
};
}


## How I learned to stop worrying and love the var

When types are obvious, the common convention is to use var. var all the things!

## Return the simplest data types possible

SetUserDetails returns a List<string>, when it should probably be returning IEnumerable<T>. If you need add/remove functionality, you can use ICollection<T>, and if you really need list functions, you can use IList<T>.

Rather than directly using the command and reader object constructors, there are methods you can use to build them. When building a command object, you can use your connection object with the IDbConnection.CreateCommand method.

## Final Version

Taking the above suggestions into account, I came up with the following for your two DAL methods:

public DataTable FillDataGrid()
{
string constr = ConfigurationManager.ConnectionStrings["dbfString"].ConnectionString;
using(var dbfCon = new MySqlConnection(constr))
{
dbfCon.Open();
using(var MyQuery = dbfCon.CreateCommand())
{
MyQuery.CommandText = "SELECT em_pplid, em_name, em_netname, em_init, em_dept FROM employs WHERE em_netname NOT LIKE '' AND em_type != 2";
var dTable = new DataTable();
return dTable;
}
}
}

public IEnumerable<UserDetails> GetUserDetails()
{
var userID = Properties.Settings.Default.UserID;
var constr = ConfigurationManager.ConnectionStrings["dbfString"].ConnectionString;
using(var dbfCon = new MySqlConnection(constr))
{
dbfCon.Open();
using(var dbfCmd = dbfCon.CreateCommand())
{
dbfCmd.CommandText = "SELECT em_surname, em_name, em_netname, em_init, em_dept, em_title FROM employs WHERE em_pplid = @userID";
{
var listUser = new List<UserDetails>();

{
var details = new UserDetails
{
};

Disclosure: I do an increasing amount of functional work lately, so I tend to use var more than others might. Feel free to put back some of the explicit type names if you feel uncomfortable leaving them as var.