2
\$\begingroup\$

How can I improve my login script and is there a cleaner way to get the same result? I had this project and redesigned it using MVP patten . Working with this patten is a new challenge for me. It's hard to tell if I've done the best work possible.

My project is composed of Models, Views, Presenter, Repository

In my program.cs I initialized my loginView:

ILoginView view = new LoginFrom();
new LoginPresentor(view, sqlConnectionString);
Application.Run((Form)view);

The login Form Shows up and I enter Username and Password and click on Singin would run this:

Presentor/LoginPresentor.cs

private void Login(object sender, EventArgs e)
    {            
        bool emptyUser = _loginView.UserName == "";
        bool emptyPassword = _loginView.Password == "";

        if(emptyUser!= true && emptyPassword != true)
        {
            IUsersRepository _userRepository = new UserRepository(_sqlConnectionString);
            _isLoggedIn = _userRepository.GetByUserAndPassword(_loginView.UserName, _loginView.Password);
            if(_isLoggedIn)
            {
                _loginView.Hide();
                IMainView view = new DashboardForm();
                new MainPresentor(view, _sqlConnectionString);
            }
        }
        
    }

what I'm doing here is create the function to check for the user and password in the database in the UserView where I can enter and delete etc.. and I add new Repository to the UserView to get the data and check if a get any data data return true else return false and use it in the Login Form to show the next View

This is the Rest of the code

Models/ IUserRepository.cs (interface)

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Inventory.Model
{
    public interface IUsersRepository
    {
        void Add(UserModel userModel);
        void Update(UserModel userModel);
        void Delete(int id);
        IEnumerable<UserModel> GetAll(); // Populate all Data
        IEnumerable<UserModel> GetByvalue(string value); // For Search
        bool GetByUserAndPassword(string name, string password); // Get Data By username and password
    }
}

_Repository/UserRepository.cs

public bool GetByUserAndPassword(string name, string password)
{
    var userList = new List<UserModel>();
    bool isLogin;
    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand())
        {
            connection.Open();
            command.Connection = connection;
            command.CommandText = @"Select * FROM Users where UserName = @name And UserPassword = @password";
            command.Parameters.Add("@name", SqlDbType.VarChar).Value = name;
            command.Parameters.Add("@password", SqlDbType.VarChar).Value = password;
            using (var reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    var userModel = new UserModel();
                    userModel.Id = (int)reader[0];
                    userModel.Name = reader[1].ToString();
                    userModel.Password = reader[2].ToString();
                    userModel.Role = reader[3].ToString();
                    userList.Add(userModel);                        
                }
            if (userList.Count > 0)
                isLogin = true;                       
            isLogin = false;
        }
    return isLogin;
}

*Note I still did not add Error message to Failed Login

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Some quick remarks:

  • Who taught you to do this: bool emptyUser = _loginView.UserName == "";? Why not use what everyone uses: string.IsNullOrEmpty()?

  • You say IUserRepository.cs is in the folder Models, despite it not being a model but a repository. But then its namespace is Inventory.Model, so that means its namespace doesn't correspond with its location (Model" vs "Models"). This is bad.

  • Don't use ADO.NET. Use a lightweight ORM like Dapper, or a full ORM like Entity Framework.

  • Don't store a password in plain text in your database. There are plenty of technologies available to securely manage users; at the very least you'll need to encrypt the password. Look into subjects like authentication and authorization.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you For these helpful notes, I Edit the way to check for empty values and for the 'Inventory.Model' I've changed the folder name seems that some of the namespace use the old Name I update it as well now all of them refer to Models \$\endgroup\$ Aug 2, 2022 at 18:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.