Database Connection Program

Question

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(); dataGrid.CanUserAddRows = false; string username = Environment.UserName; } /*** * * 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; string username = dataRow.Row.ItemArray[2].ToString(); 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(); NavigationService.Navigate(new Uri(@"View/HRSystem/HRSystem.xaml", UriKind.Relative)); } else { MessageBox.Show("Please Select a User!"); } } /*** * * 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")) { e.Column.Header = "ID"; } if (e.PropertyName.StartsWith("em_name")) { e.Column.Header = "Name"; } if (e.PropertyName.StartsWith("em_netname")) { e.Column.Header = "Net Name"; } if (e.PropertyName.StartsWith("em_init")) { e.Column.Header = "Initials"; } if (e.PropertyName.StartsWith("em_dept")) { e.Column.Header = "Department"; } } /*** * * 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; if (nNameRad.IsChecked == true) { dv.RowFilter = "em_netname LIKE '%" +searchBox.Text+ "%'"; } if (deptRad.IsChecked == true) { dv.RowFilter = "em_dept LIKE '%" + searchBox.Text + "%'"; } if (sNameRad.IsChecked == true) { dv.RowFilter = "em_name LIKE '%" + searchBox.Text + "%'"; } if (initRad.IsChecked == true) { dv.RowFilter = "em_init LIKE '%" + searchBox.Text + "%'"; } } private void AddEmployee(object sender, RoutedEventArgs e) { NavigationService.Navigate(new Uri(@"View/HRSystem/AddEmployee.xaml", UriKind.Relative)); } } 

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); OleDbDataAdapter dataAdapter = new OleDbDataAdapter(MyQuery); dataAdapter.Fill(dTable); 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); OleDbDataReader myReader = dbfCmd.ExecuteReader(); List<string> listUser = new List<string>(); while (myReader.Read()) { listUser.Add(myReader[0].ToString()); //Surname listUser.Add(myReader[1].ToString()); //Name listUser.Add(myReader[2].ToString()); //Netname listUser.Add(myReader[3].ToString()); //Initials listUser.Add(myReader[4].ToString()); //Department listUser.Add(myReader[5].ToString()); //Job Title return listUser; } } catch (OleDbException) { throw; } } return null; } } 

Answer 1

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");

Then, you add it to your command with the following:

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 { Surname = myReader[0].ToString(), //Surname Name = myReader[1].ToString(), //Name Netname = myReader[2].ToString(), //Netname Initials = myReader[3].ToString(), //Initials Department = myReader[4].ToString(), //Department JobTitle = myReader[5].ToString(), //Job Title }; listUser.Add(details); } 

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>.

Take advantage of ADO.NET interfaces

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(); var dataAdapter = new MySqlDataAdapter(MyQuery); dataAdapter.Fill(dTable); 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"; dbfCmd.Parameters.Add(new MySqlParameter("@userID", userID)); using(var myReader = dbfCmd.ExecuteReader()) { var listUser = new List<UserDetails>(); while(myReader.Read()) { var details = new UserDetails { Surname = myReader[0].ToString(), //Surname Name = myReader[1].ToString(), //Name Netname = myReader[2].ToString(), //Netname Initials = myReader[3].ToString(), //Initials Department = myReader[4].ToString(), //Department JobTitle = myReader[5].ToString(), //Job Title }; listUser.Add(details); } return listUser; } } } } 

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.