Windows Forms Log in and Dashboard

Question

I'd like to see and hear some reviews on my current WinForms Login and Dashboard code.
It's a desktop application to be used only internally and not over the web.

If you have any code suggestions - I'd be more than happy to see it and see where I can improve.

LoginScreen.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using MySql.Data.MySqlClient;
using Technical_Application.Classes;
using Technical_Application.Forms;
namespace Technical_Application
{
 public partial class Login : Form
 {
 public Login()
 {
 InitializeComponent();
 if (Properties.Settings.Default.username != string.Empty)
 {
 usernameTextBox.Text = Properties.Settings.Default.username;
 }
 }
 //Login Button
 private void loginBtn_Click_1(object sender, EventArgs e)
 {
 //MySQL connection to retrive user details into a class on succesfull log in
 using (var conn = new MySqlConnection(ConnectionString.ConnString))
 {
 conn.Open();
 //Get count, username, id, and their access id
 using (var cmd = new MySqlCommand("select count(*), username, id, access from users where username = @username and password = MD5(@password)", conn))
 {
 cmd.Parameters.AddWithValue("@username", usernameTextBox.Text);
 cmd.Parameters.AddWithValue("password", passwordTextBox.Text);
 cmd.ExecuteNonQuery();
 DataTable dt = new DataTable();
 MySqlDataAdapter da = new MySqlDataAdapter(cmd);
 da.Fill(dt);
 //Check whether user exists
 if (dt.Rows[0][0].ToString() == "1")
 {
 //If the user exist - allow to log in
 //Store the infrmation from the query to UserDetails class to be used in other forms
 UserDetails.Username = dt.Rows[0][1].ToString();
 UserDetails.userId = (int)dt.Rows[0][2];
 UserDetails.accessId = (int)dt.Rows[0][3];
 //Save the username details - for future logins
 Properties.Settings.Default.username = dt.Rows[0][1].ToString();
 Properties.Settings.Default.Save();
 //Hide this form and open the main Dashboard Form
 this.Hide();
 var dashboard = new Dashboard();
 dashboard.Closed += (s, args) => this.Close();
 dashboard.Show();
 }
 //If failed login - show message
 else
 {
 MessageBox.Show("Login failed", "Technical - Login Error", MessageBoxButtons.OK, MessageBoxIcon.Warning);
 }
 }
 }
 }
 }
}

UserDetails.cs

namespace Technical_Application.Classes
{
 public class UserDetails
 {
 private static string _username;
 private static int _userId;
 private static int _accessId;
 public static string Username
 {
 get
 {
 return _username;
 }
 set
 {
 _username = value;
 }
 }
 public static int userId
 {
 get
 {
 return _userId;
 }
 set
 {
 _userId = value;
 }
 }
 public static int accessId
 {
 get
 {
 return _accessId;
 }
 set
 {
 _accessId = value;
 }
 }
 }
}

Dashboard.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using Technical_Application.Classes;
using Technical_Application.Forms.Settings.Admin;
using Tulpep.NotificationWindow;
using MySql.Data.MySqlClient;
using Technical_Application.Forms.SubForm.FSQM;
namespace Technical_Application.Forms
{
 public partial class Dashboard : Form
 {
 public Dashboard()
 {
 InitializeComponent();
 //Show welcome message
 welcomeText.Text = $"Hello {UserDetails.Username}!";
 //Depending on the users accessID load specific content only
 if (UserDetails.accessId == 1)
 {
 }
 if (UserDetails.accessId == 2)
 {
 viewRequestedDocumentsToolStripMenuItem.Visible = false;
 adminControlToolStripMenuItem.Visible = false;
 }
 if (UserDetails.accessId == 3)
 {
 requestDocumentAmendmentToolStripMenuItem.Visible = false;
 viewRequestedDocumentsToolStripMenuItem.Visible = false;
 labelRoom.Visible = false;
 adminControlToolStripMenuItem.Visible = false;
 }
 if (UserDetails.accessId == 4)
 {
 technical.Visible = false;
 labelRoom.Visible = false;
 production.Visible = false;
 settings.Visible = false;
 welcomeText.Text = $"Hello {UserDetails.Username}! \n Your account is set as inactive.";
 }
 }
 //Check if form is already open
 private Form findForm(string text)
 {
 var found = Application.OpenForms.Cast<Form>().FirstOrDefault(f => f.Text == text);
 if (!(found is null))
 {
 found.BringToFront();
 if (found.WindowState == FormWindowState.Minimized)
 {
 found.WindowState = FormWindowState.Normal;
 }
 }
 return found;
 }
 private void requestDocumentAmendmentToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Request Document Amendment");
 if (found is null)
 {
 var newDoc = new Request_Document_Amendment();
 newDoc.Show();
 }
 }
 private void viewRequestedDocumentsToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("View Requested Documents");
 if (found is null)
 {
 var viewDoc = new ViewRequestedDocuments();
 viewDoc.Show();
 }
 }
 private void labelFormToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Label Room");
 if (found is null)
 {
 var labelForm = new Technical_Application.Forms.Label_Form___Label_Room.Main();
 labelForm.Show();
 }
 }
 private void requestLabelSignOffToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Request Label Sign Off");
 if (found is null)
 {
 var request_Label = new Request_Label_Sign_Off();
 request_Label.Show();
 }
 }
 private void labelSignOffSheetsToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Sign Off");
 if (found is null)
 {
 var labelSignOff = new LabelSignOff();
 labelSignOff.Show();
 }
 }
 private void requestLabelsToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Production");
 if (found is null)
 {
 var request_labels = new Technical_Application.Forms.Label_Form___Production.Main();
 request_labels.Show();
 }
 }
 private void adminControlToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Admin Control");
 if (found is null)
 {
 var adminControl = new Admin_Control();
 adminControl.Show();
 }
 }
 private void changePasswordToolStripMenuItem_Click(object sender, EventArgs e)
 {
 var found = findForm("Change Password - User");
 if (found is null)
 {
 var changePassword = new Technical_Application.Forms.Settings.User.changePassword();
 changePassword.Show();
 }
 }
 private void FoodSafetyQM_Click(object sender, EventArgs e)
 {
 var found = findForm("FSQM");
 if (found is null)
 {
 var FSQM = new FSQMMain();
 FSQM.Show();
 }
 }
 }
}

Answer 1

Before I begin, I want to call out something really positive. It's great that you're using parameters when reading data from the database; it's very easy to fall into the trap of using plain string concatenation, and there are liable to be severe consequences in doing so.

Capitalize public property names in UserDetails

Since these are all public properties, the general guidelines from Microsoft say they should be UserId and AccessId, not userId and accessId

Mark UserDetails as static

Because all the members are static.

Use auto-implemented properties in UserDetails

Since your fields are all private, and there's nothing special in either the getters or the setters, you should use auto-implemented properties. Auto-implemented properties can be used for static properties, and in a static class:

public static class UserDetails {
 public static string UserName {get;set;}
 public static int UserId {get;set;}
 public static int AccessId {get;set;}
}

Replace multiple if blocks which test on the same expression (accessId) with a switch in the Dashboard constructor

switch (userDetails.AccessId) {
 case 2:
 viewRequestedDocumentsToolStripMenuItem.Visible = false;
 adminControlToolStripMenuItem.Visible = false;
 break;
 case 3:
 requestDocumentAmendmentToolStripMenuItem.Visible = false;
 viewRequestedDocumentsToolStripMenuItem.Visible = false;
 labelRoom.Visible = false;
 adminControlToolStripMenuItem.Visible = false;
 break;
 case 4:
 technical.Visible = false;
 labelRoom.Visible = false;
 production.Visible = false;
 settings.Visible = false;
 welcomeText.Text = $"Hello {UserDetails.Username}! \n Your account is set as inactive.";
 break;
}

Refactor repetitive "find form or open new" code in Dashboard

You have code like the following multiple times in Dashboard.cs:

var found = findForm("Request Document Amendment");
if (found is null)
{
 var newDoc = new Request_Document_Amendment();
 newDoc.Show();
}

Firstly, do you expect there to be multiple windows of the same type with different values of the Text property? If not, I think it much more expressive to search for a form of a particular type.

Also, if you pass in type information to findForm and the form is not found, findForm can use that type information to create a new form instance of that type.

Something like this generic method:

private TForm findOrOpenForm<TForm>() where TForm : Form, new() {
 var found = Application.OpenForms.OfType<TForm>().FirstOrDefault();
 if (!(found is null)) {
 found.BringToFront();
 if (found.WindowState == FormWindowState.Minimized) {
 found.WindowState = FormWindowState.Normal;
 }
 } else {
 found = new TForm();
 found.Owner = this;
 found.Show();
 }
 return found; // This allows further actions outside of `findOrOpenForm`; you may not need it.
}

which you could then call like this:

private void requestDocumentAmendmentToolStripMenuItem_Click(object sender, EventArgs e) => 
 findOrOpenForm<Request_Document_Amendment>();

(NB. It may be possible to simplify this even further, but I think it would require some reflection.)

Consider using Add with an explicit data type instead of AddWithValue

When you use AddWithValue to add parameters, you're relying on the MySQL provider to figure out the corresponding data type based the object that's been passed in. I know that -- at least in SQL Server -- there may be performance penalties if the algorithm guesses incorrectly (link1, link2); I don't know if the same holds true for MySQL, but I would suggest checking this.

Consider using a DataReader instead of DataTable and DataAdapter

Per Microsoft:

You can use the ADO.NET DataReader to retrieve a read-only, forward-only stream of data from a database.

Since you're only retrieving a single row of data, and you're not reusing the retrieved data in any way (outside of the single row check), I think a MySqlDataReader is a better choice here.

• DeveloperLV: Hello again :) .. thanks once again for an answer. Regarding the findOrOpenForm I have an error The type or namespace name 'T' could not be found (are you missing a using directive or an assembly reference?).
• user20416: @LV98 Fixed. I'm writing all this stuff outside the IDE, so there may be some errors.
• user20416: @LV98 I've added another point, about DataReader.
• user20416: RE Is it a function or what is it? @LV98 Do you mean => (AKA "fat arrow")? Expression-bodied members is a syntax introduced in C# 6 and expanded in C# 7 that lets you use => and a single expression instead of a full-blown method body.. And yes, it is a function in this case.
• user20416: @LV98 RE .Owner -- Presumably Owner is a property on Form. I've updated findOrOpenForm to do this.