Communities

Writing
Writing
Codidact Meta
Codidact Meta
The Great Outdoors
The Great Outdoors
Photography & Video
Photography & Video
Scientific Speculation
Scientific Speculation
Cooking
Cooking
Electrical Engineering
Electrical Engineering
Judaism
Judaism
Languages & Linguistics
Languages & Linguistics
Software Development
Software Development
Mathematics
Mathematics
Christianity
Christianity
Code Golf
Code Golf
Music
Music
Physics
Physics
Linux Systems
Linux Systems
Power Users
Power Users
Tabletop RPGs
Tabletop RPGs
Community Proposals
Community Proposals
tag:snake search within a tag
answers:0 unanswered questions
user:xxxx search by author id
score:0.5 posts with 0.5+ score
"snake oil" exact phrase
votes:4 posts with 4+ votes
created:<1w created < 1 week ago
post_type:xxxx type of post
Search help
Notifications
Mark all as read See all your notifications »
Code Reviews

Welcome to Software Development on Codidact!

Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.

C# MVVM Login Project

+6
−0

I was wondering if somebody can review my code? I am creating a simple login desktop application, just to get used to the MVVM pattern using WPF.

View

<Window x:Class="Login_App.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Login_App"
        xmlns:vm ="clr-namespace:Login_App.ViewModel"
        mc:Ignorable="d"


    Title="Login" Height="380" Width="650" ResizeMode="NoResize" WindowStartupLocation="CenterScreen">
    <Window.Resources>
        <ResourceDictionary>
            <vm:LoginVM x:Key="vm"/>
        </ResourceDictionary>
    </Window.Resources>
    
    <StackPanel DataContext="{StaticResource vm}">

        <TextBlock Text="Username"/>
        <TextBox Text="{Binding Username, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
        <TextBlock Text="Password"/>
        <TextBox Text="{Binding Password, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
        <Button Content="Login"
                Command="{Binding LoginCommand}"
                CommandParameter="{Binding User}"/>

    </StackPanel>
</Window>

LoginView.xaml.cs

namespace Login_App
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        LoginVM viewModel;
        public MainWindow()
        {
            InitializeComponent();

            viewModel = Resources["vm"] as LoginVM;
            viewModel.Authenticated += ViewModel_Authenticated;
        }

        private void ViewModel_Authenticated(object sender, EventArgs e)
        {
            Close();
        }
    }
}

Model

namespace Login_App.Model
{
    public class UserModel
    {
        public int UserID { get; set; }
        public string Username { get; set; }
        public string Email { get; set; }
        public string Password { get; set; }
        public int DepartmentID { get; set; }
        public int GroupID { get; set; }
        public bool Inactive { get; set; }
    }
}

ViewModel

This is the structure:

Image alt text

LoginVM.cs

namespace Login_App.ViewModel
{
    public class LoginVM : INotifyPropertyChanged
    {
        private UserModel user;
        public UserModel User
        {
            get { return user; }
            set
            {
                user = value;
                OnPropertyChanged("User");
            }
        }

        private string username;
        public string Username
        {
            get { return username; }
            set
            {
                username = value;
                User = new UserModel
                {
                    Username = username,
                    Password = this.Password
                };
                OnPropertyChanged("Username");
            }
        }

        private string password;
        public string Password
        {
            get { return password; }
            set
            {
                password = value;
                User = new UserModel
                {
                    Username = this.Username,
                    Password = password
                };
                OnPropertyChanged("Password");
            }
        }

        public Commands.LoginCommand LoginCommand { get; set; }

        public LoginVM()
        {
            LoginCommand = new Commands.LoginCommand(this);

            user = new UserModel();
        }

        public async void Login()
        {
            bool result = await Helpers.MySQLHelper.Login(User);
            if (result)
            {
                Authenticated?.Invoke(this, new EventArgs());
            }
        }

        public EventHandler Authenticated;

        public event PropertyChangedEventHandler PropertyChanged;
        private void OnPropertyChanged(string propertyName)
        {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
        }
    }
}

Commands > LoginCommand.cs

namespace Login_App.ViewModel.Commands
{
    public class LoginCommand : ICommand
    {
        public LoginVM ViewModel { get; set; }
        public event EventHandler CanExecuteChanged
        {
            add { CommandManager.RequerySuggested += value; }
            remove { CommandManager.RequerySuggested -= value; }
        }

        public LoginCommand(LoginVM vm)
        {
            ViewModel = vm;
        }

        public bool CanExecute(object parameter)
        {
            UserModel user = parameter as UserModel;

            if (user == null)
                return false;

            if (string.IsNullOrEmpty(user.Username))
                return false;

            if (string.IsNullOrEmpty(user.Password))
                return false;

            return true;

        }

        public void Execute(object parameter)
        {
            ViewModel.Login();
        }
    }
}

Helpers > MySQLHelper.cs

public class MySQLHelper
{
    // Login Functionality
    public static async Task<bool> Login(UserModel user)
    {
        try
        {
            /// Query to get count of users from the parameters provided by the user.
            /// On successful Login, the details from the database,
            /// will be saved in the <App.xaml.cs> file.

            string mysqlUserDetails = "SELECT COUNT(*), u.id AS UserID, Username, Email, DepartmentID, Inactive" +
                " FROM users u WHERE Username = @Username AND Password = MD5(@Password)";

            using(var connection = new MySqlConnection(ConfigurationManager.ConnectionStrings["MySQL"].ConnectionString))
            {
                var userDetails = connection.Query<UserModel>(mysqlUserDetails, new { Username = user.Username, Password = user.Password }).ToList();

                // Determine whether to autherize or not
                if(userDetails.Count == 1)
                {
                    var details = userDetails.First();
                    App.UserId = details.UserID;
                    App.Username = details.Username;
                    App.Email = details.Email;
                    App.DepartmentID = details.DepartmentID;
                    App.Inative = details.Inactive;

                }


                

            }
            return false;
        }
        catch (Exception ex)
        {
            return false;
        }
    }
}
History
Why does this post require moderator attention?
You might want to add some details to your flag.
Why should this post be closed?

0 comment threads

2 answers

You are accessing this answer with a direct link, so it's being shown above all other answers regardless of its score. You can return to the normal view.

+5
−0
<Window x:Class="Login_App.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Login_App"
        xmlns:vm ="clr-namespace:Login_App.ViewModel"
        mc:Ignorable="d"

The template contains a bunch of junk. If you're not using it, delete it. I think that you can eliminate at the very least the namespaces local, mc, d. My next suggestion would also eliminate vm.


    <Window.Resources>
        <ResourceDictionary>
            <vm:LoginVM x:Key="vm"/>
        </ResourceDictionary>
    </Window.Resources>
    
    <StackPanel DataContext="{StaticResource vm}">

This feels to me like an odd way to do things. I would be more inclined to remove this and instead set DataContext = viewModel in the codebehind constructor, before InitializeComponent();. (That would make it the DataContext for the window rather than just the StackPanel, but I don't see any problem with that).


        <TextBlock Text="Username"/>
        <TextBox Text="{Binding Username, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
        <TextBlock Text="Password"/>
        <TextBox Text="{Binding Password, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
        <Button Content="Login"
                Command="{Binding LoginCommand}"
                CommandParameter="{Binding User}"/>

This should probably use PasswordBox for the password. It's a hassle to use (you have to name it and pass it as the command parameter with CommandParameter="{Binding ElementName=myPasswordBox}"), but it's pretty much expected nowadays that you don't show passwords in plain text.


    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>

Again, ditch the junk (optionally replacing it with an actually useful comment).


        private void ViewModel_Authenticated(object sender, EventArgs e)
        {
            Close();
        }

Ok, this is a proof-of-concept, but in practical application you need to be careful what assumptions other code makes on the basis of this window closing.


    public class UserModel
    {
        public int UserID { get; set; }
        public string Username { get; set; }
        public string Email { get; set; }
        public string Password { get; set; }
        public int DepartmentID { get; set; }
        public int GroupID { get; set; }
        public bool Inactive { get; set; }
    }

Is it really necessary for all of these to have public setters? That seems like a bad idea.


    public class LoginVM : INotifyPropertyChanged
    {
        private UserModel user;
        public UserModel User
        {
            get { return user; }
            set
            {
                user = value;
                OnPropertyChanged("User");

Use nameof. If your IDE didn't give you a warning here, check that your project is including the standard analysers. If it did give you a warning, configure the analysers to make it an error. The only "good" reason for a property name not to use nameof is backwards compatibility with something which had a typo.


        private string username;
        public string Username
        {
            get { return username; }
            set
            {
                username = value;
                User = new UserModel
                {
                    Username = username,
                    Password = this.Password
                };

and similarly Password.

I'm deeply suspicious of this duplication, especially because UserModel isn't immutable.

myModel.User.Username = "foo";
myModel.Password = "bar";

would do something very different if you changed the order of the two lines, and that's a trap waiting to catch someone out.

Making the UserModel class immutable would improve the situation slightly, but I'm not even convinced that the same class should expose both User and its properties. Probably the login model should have Username and Password and User should be the return value of the Login method.


        public Commands.LoginCommand LoginCommand { get; set; }

Code to the interface: is there any reason that this shouldn't have type ICommand?

Why the setter?


        public bool CanExecute(object parameter)
        {
            UserModel user = parameter as UserModel;

            if (user == null)
                return false;

            if (string.IsNullOrEmpty(user.Username))
                return false;

            if (string.IsNullOrEmpty(user.Password))
                return false;

            return true;

        }

        public void Execute(object parameter)
        {
            ViewModel.Login();
        }

Having a CanExecute which checks the parameter and an Execute which ignores the parameter is a code smell which says that this can almost certainly be refactored to be simpler and less tightly coupled.


    public static async Task<bool> Login(UserModel user)
    {
        try
        {
            /// Query to get count of users from the parameters provided > by the user.
            /// On successful Login, the details from the database,
            /// will be saved in the <App.xaml.cs> file.

            string mysqlUserDetails = "SELECT COUNT(*), u.id AS UserID, Username, Email, DepartmentID, Inactive" +
                " FROM users u WHERE Username = @Username AND Password = MD5(@Password)";

            using(var connection = new MySqlConnection(ConfigurationManager.ConnectionStrings["MySQL"].ConnectionString))
            {
                var userDetails = connection.Query<UserModel>(mysqlUserDetails, new { Username = user.Username, Password = user.Password }).ToList();

                // Determine whether to autherize or not
                if(userDetails.Count == 1)
                {
                    var details = userDetails.First();
                    App.UserId = details.UserID;
                    App.Username = details.Username;
                    App.Email = details.Email;
                    App.DepartmentID = details.DepartmentID;
                    App.Inative = details.Inactive;

                }


                

            }
            return false;
        }
        catch (Exception ex)
        {
            return false;
        }
    }
}

Why the many contiguous lines of whitespace in the middle?

The comment says that "the details from the database will be saved in the <App.xaml.cs> file". What does that mean? It sounds like it's trying to modify its own code, but it's not in an interpreted language.

There is no path in this method which returns true.

Consider using an ORM, even if it's as lightweight as Dapper.

MD5 has not been an acceptable way to hash passwords for at least the past 20 years. You should be looking at something like bcrypt, scrypt, Argon2, ..., and ideally should include an upgrade path for both algorithm and difficulty parameters. There should probably also be a random delay to reduce the side-channel information on which usernames are valid.

History
Why does this post require moderator attention?
You might want to add some details to your flag.

1 comment thread

General comments (2 comments)
+2
−0

Things you might consider to improve your code:

  1. Use nameof instead of magic strings. Example: OnPropertyChanged("User"); can be replaced with OnPropertyChanged(nameof(User));. This allows for renaming to properly work.

  2. Consider using on ORM such as Entity Framework Core instead of explicitly writing the queries. This is particularly useful for CRUD operations where EF does the tracking of changes to be persisted.

  3. Not sure, but the little image suggests that the MySql queries are placed inside the ViewModel. It is a good idea to have a clear separation between the data persistence layer and the view. You can create another project (class library?) to include all data access related functionality.

  4. Consider replacing MD5 with SHA256 or similar.

History
Why does this post require moderator attention?
You might want to add some details to your flag.

0 comment threads

Sign up to answer this question »