Database Connection Program With HR Functionality

Question

I've started writing a fully functional Database program in C# that allows a user to access a HR system (with more systems planned in the future). I have come over to C# from Java around two weeks ago, whilst I understood MVC in Java I have been recommended to use MVVC in C#, something I am struggling to understand.

Currently the program is only using DBF files (the format the current Database system is using), but in the future I will also be updating MySQL files, with the intentions of "one day" moving over completely to MySQL.

For now I would appreciate some general advice on writing clean C# code, possibly with an emphasis on how I would adapt what I have written so far into a MVVC framework. Here are the classes I have so far.

First the user logs into the database. Currently this is just checking their user input against a stored password in the DBF file (not great I appreciate);

LoginPage

namespace SDC_Database { public partial class LoginPage : Page { public LoginPage() { InitializeComponent(); } private void CheckLogin(object sender, RoutedEventArgs e) { CheckUserDetails cd = new CheckUserDetails(); int userPass = int.Parse(cd.ReturnUserPass(usernameBox.Text.ToString())); int enteredPass = int.Parse(passwordBox.Password); if (userPass == enteredPass) { MessageBox.Show("Success!"); } else { MessageBox.Show("Incorrect Password!"); } NavigationService.Navigate(new Uri(@"View/UserSelection.xaml", UriKind.Relative)); } } } 

CheckUserDetails

namespace SDC_Database.Controller { class CheckUserDetails { public string ReturnUserPass(string username) { string constr = ConfigurationManager.ConnectionStrings["dbfString"].ConnectionString; string userPass = "noPass"; using (OleDbConnection dbfCon = new OleDbConnection(constr)) { try { dbfCon.Open(); OleDbCommand dbfQuery = new OleDbCommand("SELECT em_password FROM employs WHERE em_netname = '" + username + "'", dbfCon); OleDbDataReader dReader = dbfQuery.ExecuteReader(); while (dReader.Read()) { userPass = dReader[0].ToString(); return userPass; } } catch (OleDbException) { throw; } } return userPass; } } } 

After this, the user continues into the program and displayed the main page in the HRSystem;

UserSelection

namespace SDC_Database { 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 + "%'"; } } } } 

UserSelection takes advantage of another classes functionality,

DataAcessor

namespace SDC_Database { 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(); OleDbDataReader myReader; 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); 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

I'm only going to concentrate on the code in your LoginPage class for now as I think there's enough to say about that :)

Namespaces with underscores look a bit horrible to me. I can't find anything that definitively says not to use them but I have very rarely seen them used Thanks to BCdotWEB for finding the link: DO NOT use underscores, hyphens, or any other nonalphanumeric characters..

According to the capitalization conventions on msdn:

Do capitalize only the first character of acronyms with three or more characters, except the first word of a camel-cased identifier.

That means your namespace should be SdcDatabase.

On to this code:

CheckUserDetails cd = new CheckUserDetails(); int userPass = int.Parse(cd.ReturnUserPass(usernameBox.Text.ToString())); 

What happens when usernameBox contains an invalid username? You'll either return an empty string or "noPass" - either way, the Parse will throw anyway.

The control should also be renamed to usernameTextBox.

Ideally you should create a method on a well named class that takes a username and a password and returns either true or false depending on whether the log in attempt succeeded:

public class UserAuthenticationService { public static bool AuthenticateUser(string username, string password) { // select count(*) from employs where username = username and password = password // if count == 1 => success // else => failure } } 

I'm not going to blame you for the database schema but storing passwords in plaintext is evil. Only allowing integers makes it even worse!

I'm going to assume you know about hashing passwords from your Java background - if there's any way you can implement that here - please do!

Are you aware that you're also reducing the security by parsing to an int? "01" and "0000000000001" are not the same password as "1" but your code will treat them as equal.

FYI, users don't tend to like exclamation marks in messages.

MessageBox.Show("Success!"); 

This doesn't need to be a verbatim string:

new Uri(@"View/UserSelection.xaml", UriKind.Relative) 

Forward slashes aren't significant in C# strings.

Is this a WPF app? If so you should use the Model-View-ViewModel (MVVM) pattern in preference to just MVC.

Just FYI - look up SQL injection and how to properly parameterize SQL commands in C#.

Answer 2

Why is there a ToString() here: usernameBox.Text.ToString()? Doesn't the Text property already return a string?

---

Why are you getting the stored password from the user name and then comparing that to the password the user entered in code that is in your UI? This kind of check should be happening in a back-end and return a boolean.

Moreover, this kind of check is just mindbogglingly bad: you're storing an unencrypted password, one that is an int? That's wrong on so many levels...

---

Why do you have a namespace that is called "database", and even worse, why does it contain controllers? You need to separate your Data Access Layer, your Business Logic Layer and your UI Layer.

If this is a WPF project, implement MVVM.

---

OleDbCommand, OleDbDataReader etc. all need to be inside a using statement, just like OleDbConnection.

---

What's the point of this:

catch (OleDbException) { throw; } 

Either catch it and do something with it, or don't catch it.

---

This shouldn't be in the code-behind of a Page:

DataAccessor da = new DataAccessor(); DataTable dt = da.FillDataGrid(); foreach (DataRow dr in dt.Rows) 

This should be in a DAL that returns DTOs etc. Binding a dataGrid to a dataTable is IMHO outdated, especially when you need to need to write this kind of code to sanitize the data:

if (!dr.IsNull("em_netname")) { dr["em_netname"] = dr["em_netname"].ToString().Trim(); } 

This is just begging for an ORM like NHibernate or Entity Framework.

Don't even get me started on ChangeColumnNames(): why does this method exist? This "logic" should be in your XAML, not in your code behind.

---

This code is quite scary:

DataRowView dataRow = (DataRowView)dataGrid.SelectedItem; string username = dataRow.Row.ItemArray[2].ToString(); userImage.Source = new BitmapImage(new Uri(@"S:\Picture\"+username+".jpg")); 

What if the grid changes and the userName isn't the third column? What if the images are moved from @"S:\Picture\" to another location?

And then there's this:

userImage.Source = new BitmapImage(new Uri(@"C:\Users\DanD\Desktop\noimage.png")); 

noimage.png should be part of your project and that's how you should access it.

---

I'm baffled by listUser. Why don't you have a custom class with meaningful property names? Also, in the one place where you call SetUserDetails(), you don't even do anything with what this method returns.

---

Your commenting style is non-standard and takes up waaaay too much vertical space:

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

I also feel those comments are mostly superfluous, describing things that are clear enough in the code. Abide by the rule: Code Tells You How, Comments Tell You Why.