2
\$\begingroup\$

Goal:

Goal of this application is to sign in using MySQL database and across the forms access the username and access id.

Reason I am sharing the username is for only reason at the moment, is to display a welcome message for the user.

For the access id, depending on what number id, different dashboard buttons will restricted.

I'd like to hear some of your thoughts on my coding - and whether the structure is good.

Login Screen.xaml.cs

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
    }


    //Send object from MySQL results to the User class
    User u = new User();


    private void Button_Click(object sender, RoutedEventArgs e)
    {

        try
        {


            //Connection string
            string connString = ConfigurationManager.ConnectionStrings["Technical_Application.Properties.Settings.MySQL"].ConnectionString;
            using (var conn = new MySqlConnection(connString))
            {
                conn.Open();
                //MySQL command
                using (var cmd = new MySqlCommand("SELECT count(*), id, access, username from users where username = '"+ txtUsername.Text + "' and password  = MD5('" + txtPassword.Password + "');", conn))
                {
                    cmd.ExecuteNonQuery();
                    //Place results into dataTable
                    DataTable dt = new DataTable();
                    MySqlDataAdapter da = new MySqlDataAdapter(cmd);
                    da.Fill(dt);
                    conn.Close();

                    if (dt.Rows[0][0].ToString() == "1")
                    {
                        //Send results to class
                        u.id = Int32.Parse(dt.Rows[0][1].ToString());
                        u.access = Int32.Parse(dt.Rows[0][2].ToString());
                        u.username = dt.Rows[0][3].ToString();
                        Dashboard dashboard = new Dashboard();
                        dashboard.Show();
                        this.Close();
                    }

                    else
                    {
                        MessageBox.Show("Login Failed", "Technical Login Error", MessageBoxButton.OK, MessageBoxImage.Warning);
                    }

                }
            }
        }

        catch (Exception ex) {
            MessageBox.Show(ex.ToString());
        }

    }

    private void Window_Loaded(object sender, RoutedEventArgs e)
    {
        txtUsername.Focus();
    }

    private void CloseButton_Click(object sender, RoutedEventArgs e)
    {
        Close();
    }
}

Login Screen.xaml

<Grid>
        <Grid>
            <Grid.RowDefinitions>
                <RowDefinition/>
                <RowDefinition/>
            </Grid.RowDefinitions>


            <Border CornerRadius="10" Grid.RowSpan="2">
                <Border.Background>
                    <LinearGradientBrush>
                        <GradientStop Color="White"/>
                    </LinearGradientBrush>
                </Border.Background>
            </Border>

            <Button Name="CloseButton" Content="x" Margin="0,10,0,164" Foreground="Black" FontSize="30"
                    VerticalAlignment="Center" HorizontalAlignment="Right" Height="51" BorderBrush="Transparent"
                    Background="Transparent" Click="CloseButton_Click"></Button>

            <StackPanel VerticalAlignment="Center">
                <Image Source="Resources/Logo.jpg" Width="80"/>
            </StackPanel>



            <StackPanel Grid.Row="1">
                <StackPanel Orientation="Horizontal">
                    <TextBox FontFamily="Helvetica" x:Name="txtUsername"
                     FontWeight="Light"
                     HorizontalContentAlignment="Left"
                     HorizontalAlignment="Center"
                     Width="235"
                     Foreground="Black"
                     FontSize="18"
                     Height="30"
                             Margin="63,0,0,0"
                     />
                    <iconPacks:PackIconControl Kind="{x:Static iconPacks:PackIconMaterialKind.Account}"
                           Width="24"
                           Height="24" 
                           HorizontalAlignment="Center" VerticalAlignment="Center" Margin="10,0,0,0"/>
                </StackPanel>


                <StackPanel  Orientation="Horizontal" Margin="0,30,0,0">
                    <PasswordBox FontFamily="Helvetica" x:Name="txtPassword"
                     FontWeight="Light"
                     HorizontalContentAlignment="Left"
                     HorizontalAlignment="Center"
                     Width="235"
                     Foreground="Black"
                     FontSize="18"
                     Height="30" Margin="63,0,0,0"
                     />
                    <iconPacks:PackIconControl Kind="{x:Static iconPacks:PackIconMaterialKind.FormTextboxPassword}"
                           Width="24"
                           Height="24" 
                           HorizontalAlignment="Center" VerticalAlignment="Center" Margin="10,0,0,0"/>
                </StackPanel>
                <Button Margin="0,50,0,0" Width="200" Click="Button_Click" Foreground="White">Login</Button>
            </StackPanel>




        </Grid>
    </Grid>

User.cs - This is where I store the info and shared later for other forms to access the information required.

namespace Technical_Application
{
    class User
    {
        public int id;
        public int access;
        public string username;

        public int ID
        {
            get { return id; }
            set { id = value; }
        }

        public int Access
        {
            get { return access; }
            set { access = value; }
        }

        public string Username
        {
            get { return username; }
            set { username = value; }
        }

    }
}

More detail:

This application will not be cloud or web based. It's fully only for a desktop application and to be used internally.

\$\endgroup\$
1
  • \$\begingroup\$ This code doesn't do what you describe. The User information isn't shown to be used on the other forms. \$\endgroup\$
    – user33306
    May 11, 2020 at 14:57

2 Answers 2

6
\$\begingroup\$

Some quick remarks:

  • If you're doing WPF, you should be using MVVM. There is a learning curve, but your code will be vastly easier to maintain once you add more and more features.

  • Much of the code inside Button_Click should be in separate classes (and arguably even separate layers).

  • Your code is vulnerable to SQL injection. Also, avoid writing ADO.NET and instead use Dapper.

  • Do not mix the UI and the back-end logic. Have your login logic return a custom class and display an error message depending on the contents of that class.

  • Why are the fields of User set to public? Why even have such fields, when auto-implemented properties have been a thing for more than a decade?

  • To me, Access feels like it should be an enum and not a meaningless int.

\$\endgroup\$
2
  • \$\begingroup\$ Appreciate your remarks. I have tried MVVM but struggle to understand it. I'll need to find a proper lesson or tutorial for it, if you have any beginner links, feel free to fire them at me. Regarding Button_Click - can you expand on that please? That's how I've learned it and seen others do it, i'd like to know why you suggested to separate it? Dapper great link! cheers. \$\endgroup\$ May 11, 2020 at 15:16
  • \$\begingroup\$ (Once you do MVVM, you won't even have a Button_Click method, you're working with Commands (e.g. stackoverflow.com/a/39601781/648075 ).) The separation is necessary, because otherwise you'll end up with an overlong code behind file that is impossible to maintain. If you move all that DB code to a class of its own and call that, your method will decrease in length and become easier to read. See also en.wikipedia.org/wiki/SOLID . \$\endgroup\$
    – BCdotWEB
    May 11, 2020 at 15:33
2
\$\begingroup\$

DB query

Check how you can use LINQ with mySQL. The query code will be shorter and readable with LINQ. Also, syntax errors are caught at compile time.

Regardless of my suggestion to use LINQ, here my review of your current implementation :

  • Don't ever concatenate strings to create a query. This way is vulnerable to SQL Injection. Use query parameters instead.

  • No need for count(*), just check the number of rows in the DataTable.

Error handling

You show a message box with the exception. It means nothing to the user. Also, it is considered not safe to show internal errors.

I suggest at least to add a human-friendly error like "an error occurred, please send this to XXX Exception message "

I guess you did this so the user will pass you the error.
These are the approaches I am familiar with:

  1. Write the errors in log and add a way for the user to send it to you.
  2. Use a library that reports errors to an external service.

When the user doesn't succeed to connect aka connection.Open fails. The user gets an error message of the exception.

Store User

I assume Dashboard needs the user.

I don't understand why user is a member of the form; it is not used anywhere.

Create user after the query and pass it to Dashboard.

\$\endgroup\$

Your Answer

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

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