1
\$\begingroup\$

I am creating my first business application. Unfortunately, I do not have anyone experienced in .NET or c# to review my code, and this is my first time not only using SQL, but also creating code in a professional environment. As such, I would appreciate some form of criticism / review of my User class which, so far, uses SQL the most. The code is working as intended.

The idea for this User class is to populate and store data from the Users table in our database into a User object that can be instantiated at multiple spots in the application (which is a WinForms APP). This class is also part of an effort to keep business logic separate from UI logic.

One Note: The UserSeriatim is used as a form of optimistic concurrency control. If the UserSeriatim in the database is greater than the UserSeriatim stored in memory, then someone else has updated the record while you were making edits, and your updates will be discarded.

Another Note: I will be implementing a way to encrypt the database connection string, but right now it is not encrypted. Of course, I have removed all sensitive information from my code.

User.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using RGABusinessLogic
using System.Data;
using System.Data.SqlClient;
using System.Diagnostics;

namespace RGABusinessLogic
{
    /// <summary>
    /// Class representing a User
    /// </summary>
    public class User
    {
        /// <summary>
        /// UserID of the user.
        /// </summary>
        /// <remarks>Does not update</remarks>
        public int UserID { get; private set; }

        /// <summary>
        /// The user's username
        /// </summary>
        public string Username { get; set; }

        /// <summary>
        /// The user's password in hashed form
        /// </summary>
        public string PassHash { get; set; }

        /// <summary>
        /// The user's security role
        /// </summary>
        public int RoleID { get; set; }

        /// <summary>
        /// The user's window's user name
        /// </summary>
        public string WindowsUserName { get; set; }

        /// <summary>
        /// The user's first name
        /// </summary>
        public string FirstName { get; set; }

        /// <summary>
        /// The user's last name
        /// </summary>
        public string LastName { get; set; }

        /// <summary>
        /// The user's middle name
        /// </summary>
        public string MiddleName { get; set; }

        /// <summary>
        /// The user's email
        /// </summary>
        public string Email { get; set; }

        /// <summary>
        /// The user's mobile phone number
        /// </summary>
        public string MobilePhone { get; set; }

        /// <summary>
        /// The user's active status. 1 is active, 0 is inactive
        /// </summary>
        public bool Status { get; set; }

        /// <summary>
        /// The windows user name of the user who created this user
        /// </summary>
        /// <remarks>Does not update</remarks>
        public string CreateBy { get; private set; }

        /// <summary>
        /// The date this user was created
        /// </summary>
        /// <remarks>Does not update</remarks>
        public DateTime CreateDate { get; private set; }

        /// <summary>
        /// The windows user name of the user who last modified this user
        /// </summary>
        public string ModifyBy { get; private set; }

        /// <summary>
        /// The date this user was last modified
        /// </summary>
        public DateTime ModifyDate { get; private set; }

        /// <summary>
        /// The seriatim for the user
        /// </summary>
        public int UserSeriatim { get; private set; }

        /// <summary>
        /// Whether or not this user is a new user
        /// </summary>
        private bool _newUser;

        /// <summary>
        /// Whether or not this user is being modified
        /// </summary>
        private bool _modifyUser;

        /// <summary>
        /// Creates a new instance of a user
        /// </summary>
        /// <param name="newUser">Whether or not this user is a new user</param>
        /// <param name="modifyUser">Whether or not this user is being modified</param>
        /// <param name="userRecord">The data used to populate the current user if the user is not null</param>
        public User(bool newUser, bool modifyUser, DataRow userRecord = null)
        {
            _newUser = newUser;
            _modifyUser = modifyUser;
            if (_newUser)
            {
                SetCreateProperties();
            }
            else if (userRecord != null)
            {
                PopulateUserProperties(userRecord, _modifyUser);
            }
        }

        /// <summary>
        /// Sets the CreatedBy and CreateDate properties for a new user
        /// </summary>
        private void SetCreateProperties()
        {
            CreateBy = CurrentUser.WindowsUserName;
            CreateDate = DateTime.Now;
            UserSeriatim = 0;
        }

        /// <summary>
        /// Populates all user properties using the given data row, and then sets the ModifiedBy and Modify Date properties
        /// </summary>
        /// <remarks>
        /// The given data row must have field names that match the field names in the method. If this is to ever change, this method will need an update.
        /// </remarks>
        /// <param name="row">The data row used to populate this object's properties</param>
        private void PopulateUserProperties(DataRow row, bool modify)
        {
            try
            {
                UserID = ConvertFromDB<int>(row["UserID"]);
                Username = ConvertFromDB<string>(row["UserName"]);
                PassHash = ConvertFromDB<string>(row["PassHash"]);
                RoleID = ConvertFromDB<int>(row["RoleID"]);
                WindowsUserName = ConvertFromDB<string>(row["WindowsUserName"]);
                FirstName =  ConvertFromDB<string>(row["FirstName"]);
                LastName =  ConvertFromDB<string>(row["LastName"]);
                MiddleName = ConvertFromDB<string>(row["MiddleName"]);
                MobilePhone =  ConvertFromDB<string>(row["MobilePhone"]);
                Email = ConvertFromDB<string>(row["Email"]);
                Status = ConvertFromDB<bool>(row["Status"]);
                CreateBy = ConvertFromDB<string>(row["CreateBy"]);
                CreateDate = ConvertFromDB<DateTime>(row["CreateDate"]);
                UserSeriatim = ConvertFromDB<int>(row["UserSeriatim"]);
                if (modify)
                {
                    ModifyBy = CurrentUser.WindowsUserName;
                    ModifyDate = DateTime.Now;
                }
                else
                {
                    ModifyBy = ConvertFromDB<string>(row["ModifyBy"]);
                    ModifyDate = ConvertFromDB<DateTime>(row["ModifyDate"]);
                }
            }
            catch(Exception e)
            {
                Debug.WriteLine(e.ToString()); //This also seems like a good opportunity to display an error to the end user.
                return;
            }
        }

        /// <summary>
        /// Updates the user's record in the database
        /// </summary>
        /// <returns>True if record was successfully updated</returns>
        public bool UpdateUserRecord(out string errorString)
        {
            //TODO: Encryption on database username and database password
            string connectionString = @"You're not allowed to see this connection string!";

            var sqlCommand = new SqlCommand();
            var commandString = "UPDATE Users SET " +
                "Username = @Username, " +
                "RoleID = @SecurityRole, " +
                "WindowsUserName = @WindowsUsername, " +
                "FirstName = @FirstName, " +
                "LastName = @LastName, " +
                "MiddleName = @MiddleName, " +
                "Email = @Email, " +
                "MobilePhone = @MobilePhone, " +
                "Status = @Status, " +
                "ModifyBy = @ModifyBy, " +
                "ModifyDate = @ModifyDate, " +
                "UserSeriatim = @UserSeriatim " +
                "WHERE UserID = @UserID";

            sqlCommand.CommandText = commandString;

            sqlCommand.Parameters.AddWithValue("Username", Username);
            //sqlCommand.Parameters.AddWithValue("SecurityRole", (int)SecurityRole);
            sqlCommand.Parameters.AddWithValue("SecurityRole", RoleID);
            sqlCommand.Parameters.AddWithValue("WindowsUserName", WindowsUserName);
            sqlCommand.Parameters.AddWithValue("FirstName", FirstName);
            sqlCommand.Parameters.AddWithValue("LastName", LastName);
            sqlCommand.Parameters.AddWithValue("MiddleName", MiddleName);
            sqlCommand.Parameters.AddWithValue("Email", Email);
            sqlCommand.Parameters.AddWithValue("MobilePhone", MobilePhone);
            sqlCommand.Parameters.AddWithValue("Status", Status);
            sqlCommand.Parameters.AddWithValue("ModifyBy", ModifyBy);
            sqlCommand.Parameters.AddWithValue("ModifyDate", ModifyDate);
            sqlCommand.Parameters.AddWithValue("UserID", UserID);
            sqlCommand.Parameters.AddWithValue("UserSeriatim", UserSeriatim + 1);
            if (!CheckUserSeriatim(connectionString, out string modifyBy))
            {
                errorString = $"Could not Update: Record was updated by {modifyBy}";
                return false;
            }
            errorString = null;
            return RunSqlNonQuery(connectionString, sqlCommand);
        }

        /// <summary>
        /// Updates the user's password in the database
        /// </summary>
        /// <returns>True if record was successfully updated</returns>
        public bool UpdatePassHash(out string errorString)
        {
            string connectionString = @"You're not allowed to see this connection string!";

            var sqlCommand = new SqlCommand();
            var commandString = "UPDATE Users SET PassHash = @PassHash WHERE UserID = @UserID";

            sqlCommand.CommandText = commandString;

            sqlCommand.Parameters.AddWithValue("PassHash", PassHash);
            sqlCommand.Parameters.AddWithValue("UserID", UserID);
            if (!CheckUserSeriatim(connectionString, out string modifyBy))
            {
                errorString = $"Could not Update: Record was updated by {modifyBy}";
                return false;
            }
            errorString = null;
            return RunSqlNonQuery(connectionString, sqlCommand);
        }

        /// <summary>
        /// Adds the user's record to the database
        /// </summary>
        /// <returns>True if record was successfully added</returns>
        public bool AddUserRecord()
        {
            //TODO: Encryption on database username and database password
            string connectionString = @"You're not allowed to see this connection string!"; //Would I want to create a private static variable to hold the connection string?

            ModifyBy = CreateBy;
            ModifyDate = CreateDate;

            var sqlCommand = new SqlCommand();

            string commandString = 
                $"INSERT INTO Users " +
                $"(UserSeriatim, UserName, PassHash, RoleID, " +
                $"WindowsUserName, LastName, FirstName, " +
                $"MiddleName, Email, MobilePhone, " +
                $"Status, CreateBy, " +
                $"CreateDate, ModifyBy, ModifyDate) " +
                $"VALUES (@UserSeriatim, @Username, @PassHash, @SecurityRole, " +
                $"@WindowsUserName, @LastName, @FirstName, " +
                $"@MiddleName, @Email, @MobilePhone, @Status, " +
                $"@CreateBy, @CreateDate, @ModifyBy, @ModifyDate);";

            sqlCommand.CommandText = commandString;

            sqlCommand.Parameters.AddWithValue("UserSeriatim", UserSeriatim);
            sqlCommand.Parameters.AddWithValue("Username", Username);
            sqlCommand.Parameters.AddWithValue("PassHash", PassHash);
            //sqlCommand.Parameters.AddWithValue("SecurityRole", (int)SecurityRole);
            sqlCommand.Parameters.AddWithValue("SecurityRole", RoleID);
            sqlCommand.Parameters.AddWithValue("WindowsUserName", WindowsUserName);
            sqlCommand.Parameters.AddWithValue("LastName", LastName);
            sqlCommand.Parameters.AddWithValue("FirstName", FirstName);
            sqlCommand.Parameters.AddWithValue("MiddleName", MiddleName);
            sqlCommand.Parameters.AddWithValue("Email", Email);
            sqlCommand.Parameters.AddWithValue("MobilePhone", MobilePhone);
            sqlCommand.Parameters.AddWithValue("Status", Status);
            sqlCommand.Parameters.AddWithValue("CreateBy", CreateBy);
            sqlCommand.Parameters.AddWithValue("CreateDate", CreateDate);
            sqlCommand.Parameters.AddWithValue("ModifyBy", ModifyBy);
            sqlCommand.Parameters.AddWithValue("ModifyDate", ModifyDate);
 
            return RunSqlNonQuery(connectionString, sqlCommand); 
        }

        /// <summary>
        /// Tries to execute a sql non query.
        /// </summary>
        /// <param name="connectionString">Connection string to the database</param>
        /// <param name="commandString">Sql command to execute</param>
        /// <returns>True if the execution was successful</returns>
        private bool RunSqlNonQuery(string connectionString, SqlCommand command)
        {
            SqlConnection connection = null;
            try
            {
                connection = new SqlConnection(connectionString);
                connection.Open();
                command.Connection = connection;
                command.ExecuteNonQuery();
            }
            catch (SqlException e)
            {
                connection.Close();
                Debug.WriteLine(e.ToString());
                return false;
            }
            connection.Close();
            return true;
        }

        /// <summary>
        /// Checks the current UserSeriatim in the database against this object's UserSeriatim, and returns if the check passes.
        /// </summary>
        /// <param name="connectionString">Connection string to the database</param>
        /// <returns>True if the user seriatim is not greater than the current user seriatim</returns>
        private bool CheckUserSeriatim(string connectionString, out string modifiedBy)
        {
            SqlConnection connection = null;
            try
            {
                connection = new SqlConnection(connectionString);
                connection.Open();
                var commandString = "SELECT UserSeriatim, ModifyBy FROM Users WHERE UserID = @UserID";
                SqlCommand command = new SqlCommand(commandString, connection);
                command.Parameters.AddWithValue("UserID", UserID);
                SqlDataAdapter dataAdapter = new SqlDataAdapter(command);
                DataTable table = new DataTable();
                dataAdapter.Fill(table);
                if (ConvertFromDB<int>(table.Rows[0]["UserSeriatim"]) > UserSeriatim)
                {
                    modifiedBy = ConvertFromDB<string>(table.Rows[0]["ModifyBy"]);
                    return false;
                }
                connection.Close();
                modifiedBy = null;
                return true;
            }
            catch (SqlException e)
            {
                connection.Close();
                Debug.WriteLine(e.ToString());
                modifiedBy = null;
                return false;
            }
        }

        /// <summary>
        /// Sets the user's status to inactive
        /// </summary>
        public void DeactiveateUser()
        {
            Status = false;
        }

        /// <summary>
        /// Sets the user's status to active
        /// </summary>
        public void ActivateUser()
        {
            Status = true;
        }

        /// <summary>
        /// Converts an objects database value to a c# value
        /// </summary>
        /// <typeparam name="T">Type to covert to</typeparam>
        /// <param name="obj">Object to convert</param>
        /// <returns>Converted Object</returns>
        private T ConvertFromDB<T>(object obj)
        {
            if (obj == null || obj == DBNull.Value)
            {
                return default(T);
            }
            else
            {
                return (T)obj;
            }
        }
    }
}

Just incase it is important to context, here are a couple examples of where a User instance is used...

CreateUser() in CreateUserControl.cs Winforms user control

/// <summary>
        /// Creates a new user and displays a message box
        /// </summary>
        private void CreateUser()
        {
            _newUser.Username = usernameTextEdit.Text;
            _newUser.PassHash = StringHasher.ComputeSHA256Hash(passwordTextEdit.Text);
            _newUser.FirstName = firstNameTextEdit.Text;
            _newUser.LastName = usernameTextEdit.Text;
            _newUser.MiddleName = middleNameTextEdit.Text;
            _newUser.Email = emailTextEdit.Text;
            _newUser.MobilePhone = mobilePhoneTextEdit.Text;
            _newUser.WindowsUserName = windowsUserNameTextEdit.Text;
            _newUser.Status = statusCheckBox.Checked;
            try
            {
                _newUser.AddUserRecord();
                MessageBox.Show($"Created User: {_newUser.Username}");
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.ToString());
            }
        }

UpdateUser() in EditUserControl.cs Winforms User control

/// <summary>
        /// Updates the current user and displays a message box
        /// </summary>
        private void UpdateUser()
        {
            _user.Username = usernameTextEdit.Text;
            _user.FirstName = firstNameTextEdit.Text;
            _user.LastName = lastNameTextEdit.Text;
            _user.MiddleName = middleNameTextEdit.Text;
            _user.Email = emailTextEdit.Text;
            _user.MobilePhone = mobilePhoneTextEdit.Text;
            _user.WindowsUserName = windowsUserNameTextEdit.Text;
            _user.Status = statusCheckBox.Checked;
            try
            {
                if (_user.UpdateUserRecord(out string errorString))
                {
                    MessageBox.Show($"Updated User: {_user.Username}");
                }
                else
                {
                    MessageBox.Show(errorString);
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.ToString());
            }
        }
\$\endgroup\$
9
  • 2
    \$\begingroup\$ 1. Use Dapper. 2. Properly handle IDisposable objects. \$\endgroup\$
    – BCdotWEB
    Jun 17 at 6:31
  • \$\begingroup\$ Is there any particular reason why did you implement your data access with ADO.NET? \$\endgroup\$ Jun 17 at 6:34
  • \$\begingroup\$ @PeterCsala It seemed straight forward to implement into the code, and it is free. All it takes is a "using system.Data.SqlClient;" Is there a good reason not to use ADO.NET? As I mentioned in the post, this is my first time working with SQL, so it is all a learning experience for me. \$\endgroup\$
    – EricNie
    Jun 17 at 13:09
  • 1
    \$\begingroup\$ ADO.NET is kinda low level compared to any modern ORM systems. That's why is it more flexible and you can get better performance. On the other hand it is tedious. Most the ORMs provide a lot of utilities out of the box. You just have to configure it and you can change tracking, object validation, migration, scaffolding, etc. \$\endgroup\$ Jun 17 at 13:15
  • 1
    \$\begingroup\$ aside from using ORM, you didn't validate user inputs, you just take the inputs as is and reassign them into the model. You should implement the proper validations for each property (username, email ..etc.) before you save it to the database. \$\endgroup\$
    – iSR5
    Jun 17 at 22:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.