2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ 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? \$\endgroup\$ – SuperBiasedMan Oct 12 '15 at 13:19
  • \$\begingroup\$ @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. \$\endgroup\$ – CBR Oct 12 '15 at 13:22
  • 1
    \$\begingroup\$ @CBreeze You can ask for both accounts to be merged: codereview.stackexchange.com/help/merging-accounts \$\endgroup\$ – BCdotWEB Oct 12 '15 at 13:52
  • \$\begingroup\$ @BCdotWEB Fantastic I have merged accounts. \$\endgroup\$ – CBR Oct 12 '15 at 14:02
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I appreciate all your feedback and will work on it all now. Just some clarity for the first point you make - I'm currently using OleDB because the old database is using .DBF files (the old database is written in VFP). I will be moving it over to MySQL in the future which is why I included it but that probably caused some confusion. \$\endgroup\$ – CBR Oct 13 '15 at 7:05
  • \$\begingroup\$ @CBR I see - I saw the mysql tag and thought you were using OleDb because you didn't realize the library existed. The only thing you may need to change aside from which set of objects you use would be how the parameterized queries work - I am not entirely sure how OleDb designates query parameters. \$\endgroup\$ – Dan Lyons Oct 13 '15 at 15:04

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.