Как улучшить код подключения к БД и установления Роли?

Я подключаюсь к БД и провожу запрос и установление Роли. Мне не нравится как громозко это выглядит, и яб хотел как то это улучшить, но я не знаю как...

public partial class Login : Form
    {
        private int x = 0;
        private int y = 0;
        public Login()
        {
            InitializeComponent();

        }
        int i;
        string status1;
        string status2;
        string status3;


        private void button1_Click(object sender, EventArgs e)
        {
            checkedRadioButton();
            switch (i)
            {
                case 1:
                    string loginUSER = textBox1.Text;
                    string passUSER = textBox2.Text;
                    DB database = new DB();

                    DataTable table = new DataTable();

                    MySqlDataAdapter adapter = new MySqlDataAdapter();

                    MySqlCommand command = new MySqlCommand("SELECT * FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP AND `Status`= @uR", database.getCon());
                    command.Parameters.Add("@uL", MySqlDbType.VarChar).Value = loginUSER;
                    command.Parameters.Add("@uP", MySqlDbType.VarChar).Value = passUSER;
                    command.Parameters.Add("@uR", MySqlDbType.VarChar).Value = status1;

                    adapter.SelectCommand = command;
                    adapter.Fill(table);

                    if (table.Rows.Count > 0)
                    {
                        MessageBox.Show("Вы вошли!");
                        Form f = new MainForm(i);
                        f.Show();
                        Hide();
                    }
                    else
                    {
                        MessageBox.Show("Проверьте корректность введенных данных");
                    }
                    break;
                case 2:
                    string loginUSER1 = textBox1.Text;
                    string passUSER1 = textBox2.Text;

                    DB database1 = new DB();

                    DataTable table1 = new DataTable();

                    MySqlDataAdapter adapter1 = new MySqlDataAdapter();

                    MySqlCommand command1 = new MySqlCommand("SELECT * FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP AND `Status`= @uR", database1.getCon());
                    command1.Parameters.Add("@uL", MySqlDbType.VarChar).Value = loginUSER1;
                    command1.Parameters.Add("@uP", MySqlDbType.VarChar).Value = passUSER1;
                    command1.Parameters.Add("@uR", MySqlDbType.VarChar).Value = status2;

                    adapter1.SelectCommand = command1;
                    adapter1.Fill(table1);

                    if (table1.Rows.Count > 0)
                    {
                        MessageBox.Show("Вы вошли!");
                        Form f = new MainForm(i);
                        f.Show();
                        Hide();
                    }
                    else
                    {
                        MessageBox.Show("Проверьте корректность введенных данных");
                    }
                    break;
                case 3:
                    string loginUSER2 = textBox1.Text;
                    string passUSER2 = textBox2.Text;

                    DB database2 = new DB();

                    DataTable table2 = new DataTable();

                    MySqlDataAdapter adapter2 = new MySqlDataAdapter();

                    MySqlCommand command2 = new MySqlCommand("SELECT * FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP AND `Status`= @uR", database2.getCon());
                    command2.Parameters.Add("@uL", MySqlDbType.VarChar).Value = loginUSER2;
                    command2.Parameters.Add("@uP", MySqlDbType.VarChar).Value = passUSER2;
                    command2.Parameters.Add("@uR", MySqlDbType.VarChar).Value = status3;

                    adapter2.SelectCommand = command2;
                    adapter2.Fill(table2);

                    if (table2.Rows.Count > 0)
                    {
                        MessageBox.Show("Вы вошли!");
                        Form f = new MainForm(i);
                        f.Show();
                        Hide();

                    }
                    else
                    {
                        MessageBox.Show("Проверьте корректность введенных данных");
                    }
                    break;

                   
            }
        }
        private void checkedRadioButton()
        {
            if (radioButton1.Checked == true)
            {
                i = 1;
                status1 = "Администратор";

            }
            if (radioButton2.Checked == true)
            {
                i = 2;
                status2 = "Мастер";
            }
            if (radioButton3.Checked == true)
            {
                i = 3;
                status3 = "Клиент";
            }
        }

Ответы (2 шт):

Автор решения: aepot

По порядку. Сначала вычищаю то что есть не меняя логики, затем даю советы по исправлениям.

Это вообще зачем здесь?

private int x = 0;
private int y = 0;

Удаляем.

Совершенно непонятно, что это и для чего это

int i;
string status1;
string status2;
string status3;

Называйте переменные более понятными именами. Почему 3 статуса? Ведь далее по логике видно, что хватит одного.

Далее идет вызов метода checkedRadioButton где видно, что каждый из этих статусов используется однократно. Тогда зачем их три? Давайте так.

private int role;
private string roleName;

Так, стоп, а зачем они вообще? :) Ведь их можно не хранить в форме, а возвращать из метода. Но 2 значения из метода возвращать не очень удобно, почему бы не создать массив, он прекрасно ложится на логику.

private readonly string[] roleNames = new[] { "Администратор", "Мастер", "Клиент" };

Единственное расхождение, это у вас нумерация ролей начинается с единицы, а у массива индексы начинаются с нуля.

Тогда метод СheckedRadioButton, кстати, называйте методы с большой буквы, будет выглядеть так.

private int СheckedRadioButton()
{
    if (radioButton1.Checked)
        return 1;
    if (radioButton2.Checked)
        return 2;
    if (radioButton3.Checked)
        return 3;
    return 0;
}

И напишу вспомогательный метод

private string GetRoleName(int role)
{
    return roleNames[role - 1];
}

Далее, запрос в базу. Вам же не нужны данные из таблицы, а нужно только посчитать количество строк в ней по данному запрос, ну так давайте считать.

SELECT COUNT(*) FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP AND `Status`= @uR

И результат запроса можно получить вот так

long rowsCount = (long)command.ExecuteScalar();

Ничего лишнего.

Далее, вы не диспозите IDisposable объекты, это не очень хорошо.

using(MySqlCommand command = new MySqlCommand("...", database.getCon())
{
    // здесь работа с командой
}

Почитайте: Использование объектов, реализующих IDisposable.

Далее, вы вызываете Hide();, а когда главная форма будет закрыта, кто закроет форму логина? В результате ваше приложение никогда не завершится. Придется использовать костыли типа Application.Exit() и прочие. Подпишитесь на событие закрытия формы и закрывайте форму логина вместе с ней.

f.FormClosed += (s, e) => Close();

Если вдруг захочется при закрытии главной формы показать форму логина, просто Close замените на Show. Всё, теперь можно из главной формы выходить просто ее закрывая через Close().

Итого получилось вот так:

public partial class Login : Form
{
    private readonly string[] roleNames = new[] { "Администратор", "Мастер", "Клиент" };

    public Login()
    {
        InitializeComponent();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        int role = СheckedRadioButton();
        string loginUSER = textBox1.Text;
        string passUSER = textBox2.Text;
        DB database = new DB();

        long rowsCount = 0;
        try
        {
            using (MySqlCommand command = new MySqlCommand("SELECT COUNT(*) FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP AND `Status`= @uR", database.getCon()))
            {
                command.Parameters.Add("@uL", MySqlDbType.VarChar).Value = loginUSER;
                command.Parameters.Add("@uP", MySqlDbType.VarChar).Value = passUSER;
                command.Parameters.Add("@uR", MySqlDbType.VarChar).Value = GetRoleName(role);
                rowsCount = (long)command.ExecuteScalar();
            }
        }
        catch (NullReferenceException) { }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
            return;
        }

        if (rowsCount > 0)
        {
            MessageBox.Show("Вы вошли!");
            Form f = new MainForm(role);
            f.FormClosed += (s, e) => Close();
            f.Show();
            Hide();
        }
        else
        {
            MessageBox.Show("Проверьте корректность введенных данных");
        }
    }

    private string GetRoleName(int role)
    {
        return roleNames[role - 1];
    }

    private int СheckedRadioButton()
    {
        if (radioButton1.Checked)
            return 1;
        if (radioButton2.Checked)
            return 2;
        if (radioButton3.Checked)
            return 3;
        return 0;
    }
}

Повторяющийся код - индикатор того что что-то пошло не так, избегайте повторяющегося кода.


Теперь замечания по общей логике:

  1. Никогда не храните в базе данных пароли в открытом виде, используйте хэширование, любое, например SHA256. Если база данных попадет в руки злодея, он по хэшам не сможет узнать оригинальные пароли.
  2. Почему роли у вас захардкожены? Заведите в базе таблицу roles с полями id, access_level, name и запрашивайте SELECT access_level, name FROM roles и не придется держать массив. К тому же, если у вас новая роль появится, вы будете форму логина перепиливать, а потом еще 10 местах код приложения изменять? Позже научитесь делать JOIN и вам еще больше понравится наличие отдельной таблицы с ролями.

Добавьте больше гибкости, закладывайте то, что может случиться в архитектуру заранее. Меньше хардкода, меньше сввязей между компонентами приложения - больше гибкости в разработке.

Кстати, я бы убрал radioButton и получал бы роль из базы, примерно так.

SELECT Status FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP

Пусть база данных определяет роль, а не пользователь.

→ Ссылка
Автор решения: Volodymyr Osadchuk

В Вашем коде нарушен принцип DRY (читайте здесь)
Вы можете написать вот так:

public partial class Login
{
    private int x = 0;
    private int y = 0;

    private string _status;

    public Login()
    {
    }

    private void button1_Click(object sender, EventArgs e)
    {
        checkedRadioButton();

        string loginUSER = textBox1.Text;
        string passUSER = textBox2.Text;
        DB database = new DB();

        DataTable table = new DataTable();

        MySqlDataAdapter adapter = new MySqlDataAdapter();

        MySqlCommand command = new MySqlCommand("SELECT * FROM `authorized` WHERE `Login` = @uL AND `Password` = @uP AND `Status`= @uR", database.getCon());
        command.Parameters.Add("@uL", MySqlDbType.VarChar).Value = loginUSER;
        command.Parameters.Add("@uP", MySqlDbType.VarChar).Value = passUSER;
        command.Parameters.Add("@uR", MySqlDbType.VarChar).Value = _status;

        adapter.SelectCommand = command;
        adapter.Fill(table);

        if (table.Rows.Count > 0)
        {
            MessageBox.Show("Вы вошли!");
            Form f = new MainForm(i);
            f.Show();
            Hide();
        }
        else
        {
            MessageBox.Show("Проверьте корректность введенных данных");
        }
    }
    private void checkedRadioButton()
    {
        if (radioButton1.Checked == true)
        {
            _status = "Администратор";
        }
        if (radioButton2.Checked == true)
        {
            _status = "Мастер";
        }
        if (radioButton3.Checked == true)
        {
            _status = "Клиент";
        }
    }
//...

И пожалуйста, посмотрите Code Convension по названию переменных и методов в C#

→ Ссылка