Рефакторинг программы

458
04 января 2017, 00:49

Я написал программу,которая рандомным образом выбирает глагол на русском языке из текстового документа и показывает нам его. Задача пользователя состоит в том,что он должен ввести в текстбоксы этот же глагол в 1,2,3 формах английского языка. Собственно сам код:

namespace Verbs2
{
   public partial class MainWindow : Window
    {
        int randomNum = 0;
        int counterWin = 0;
        int counterLose = 0;
        bool ifChecked = false;
        public MainWindow()
        {
            InitializeComponent();
        }
        public class Verbs
        {
            /*класс создан для работы над глаголами и текстового файла*/
            string[] readText = File.ReadAllLines(@"C:\Users\Рустем\Desktop\проекты\Verbs2\Verbs2\verbs.txt",
                System.Text.Encoding.GetEncoding(1251));//считываем все строки в массив
            List<string[]> res = new List<string[]>();
            public Verbs()
            {
                foreach (var line in readText)//перебираем строки массива
                {
                    res.Add(line.Split('.'));//Каждую строку сплитим и помещаем в список массивов.
                }
            }
            public string GetVerb(int a)//метод позволяет достать из листа глагол на русском языке(в 3 столбце находятся русские глаголы)
            {
                return res[a][3];
            }
            public string GetVerb(int a, int b)
            {
                return res[a][b];
            }
            public bool CheckVerbs(string a, string b, string c, int d)//проверяем соответствуют ли введенные данные,той строке что мы выбрали
            {
                if ((a.Equals(res[d][0])) && (b.Equals(res[d][1])) && (c.Equals(res[d][2])))
                    return true;
                else return false;
            }
        }
        public void SetRandom()
        {
            /*в нашем текстовом документе 68 глаголов,поэтому мы выбираем один рандомный глагол из 68*/
            Random rnd = new Random();
            randomNum = rnd.Next(0, 68);
        }
        public int GetRandom()
        {
            return randomNum;
        }
        private void btnstart_Click(object sender, RoutedEventArgs e)
        {
            /*при нажатии на кнопку "начать" создаем объект класса verb, выбираем рандомной глагол, вставляем его в lblverb2 и делаем доступной кнопку btncheck*/
            Verbs verb = new Verbs();
            SetRandom();
            lblverb2.Content = verb.GetVerb(GetRandom());
            btncheck.IsEnabled = true;
            counterWin = 0;
            counterLose = 0;
        }
        private void btncheck_Click(object sender, RoutedEventArgs e)
        {
            /* в лэйблы 4-6 вставляем глаголы на английском языке
             проверяем соответсвуют ли введенные в текстбоксы глаголы,выбранному глаголу
             делаем доступной кнопку btnnext*/
            Verbs verb = new Verbs();
            lblverb4.Content = verb.GetVerb(GetRandom(), 0);
            lblverb5.Content = verb.GetVerb(GetRandom(), 1);
            lblverb6.Content = verb.GetVerb(GetRandom(), 2);
            if (verb.CheckVerbs(textverb1.Text, textverb2.Text, textverb3.Text, GetRandom()))
            {
                lborder.BorderBrush = Brushes.Green;
                counterWin++;
                lblcw.Content = counterWin.ToString();
            }
            else
            {
                lborder.BorderBrush = Brushes.Red;
                counterLose++;
                lblcl.Content = counterLose.ToString();
            }
            btnnext.IsEnabled = true;
            btncheck.IsEnabled = false;
            ifChecked = true;
        }
        private void btnnext_Click(object sender, RoutedEventArgs e)
        {
            /*по сути повторяет действия кнопки btnstart,но при этом очищает лэйблы и текстбоксы*/
            Verbs verb = new Verbs();
            SetRandom();
            lblverb2.Content = verb.GetVerb(GetRandom());
            btncheck.IsEnabled = true;
            if (ifChecked == false)
                counterLose++;
            lblcl.Content = counterLose.ToString();
            ifChecked = false;
            lblverb4.Content = "";
            lblverb5.Content = "";
            lblverb6.Content = "";
            textverb1.Text = "";
            textverb2.Text = "";
            textverb3.Text = "";
            lborder.BorderBrush = Brushes.White;
        }
    }
}

Код XAML:

<Window x:Class="Verbs2.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="Неправильные глаголы" Height="350" Width="360">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="25"/>
            <RowDefinition />
            <RowDefinition />
            <RowDefinition />
            <RowDefinition />
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="120*" />
            <ColumnDefinition Width="120*" />
            <ColumnDefinition Width="52*" />
            <ColumnDefinition Width="24*" />
            <ColumnDefinition Width="15*" />
            <ColumnDefinition Width="24*" />
        </Grid.ColumnDefinitions>
        <!--меню-->
        <Menu  Grid.Column="0" Grid.ColumnSpan="3" Background="White">
            <MenuItem Header="File">
                <MenuItem Header="New Project" ></MenuItem>
                <Separator/>
                <MenuItem Header="Exit" ></MenuItem>
            </MenuItem>
        </Menu>
        <!---->
        <Label x:Name="lblverb1" Grid.Column="0" Grid.Row="1">Глагол:</Label>
         <Label x:Name="lblverb2" Grid.Column="1" Grid.Row="1"></Label>
        <Label x:Name="lblverb3" Grid.Column="2" Grid.Row="1" Grid.ColumnSpan="1">Счетчик</Label>
        <!--лейблы для глаголов на английском языке-->
        <Label x:Name="lblverb4" Grid.Column="0" Grid.Row="2"></Label>
        <Label x:Name="lblverb5" Grid.Column="1" Grid.Row="2"></Label>
        <Label x:Name="lblverb6" Grid.Column="2" Grid.Row="2" Grid.ColumnSpan="4"></Label>
        <!--лейблы для набора текста пользователем -->
        <TextBox x:Name="textverb1" Grid.Column="0" Grid.Row="3"></TextBox>
        <TextBox x:Name="textverb2" Grid.Column="1" Grid.Row="3"></TextBox>
        <TextBox x:Name="textverb3" Grid.Column="2" Grid.Row="3" Grid.ColumnSpan="4"></TextBox>
        <Button x:Name="btnstart" Grid.Column="0" Grid.Row="4" Click="btnstart_Click">Начать</Button>
        <Button x:Name="btncheck"  IsEnabled="False" Grid.Column="1" Grid.Row="4" Click="btncheck_Click">Проверить</Button>
        <Button x:Name="btnnext"  IsEnabled="False" Grid.Column="2" Grid.Row="4" Grid.ColumnSpan="4" Click="btnnext_Click">Следующий</Button>

        <Label x:Name="lblcw" Grid.Column="3" Grid.Row="1">0</Label>
        <Label x:Name="lbllblslash" Grid.Column="4" Grid.Row="1">/</Label>
        <Label x:Name="lblcl" Grid.Column="5" Grid.Row="1">0</Label>
        <Border x:Name="lborder" BorderBrush="White" BorderThickness="4" HorizontalAlignment="Left" Height="64" VerticalAlignment="Top" Width="360" Grid.Row="3" Grid.ColumnSpan="6"/>
        <Border x:Name="lborder1" BorderBrush="Black" BorderThickness="1" HorizontalAlignment="Left" Height="74" VerticalAlignment="Top" Width="352" Grid.Row="1" Grid.ColumnSpan="6"/>
        <Border x:Name="lborder2" BorderBrush="Black" BorderThickness="1"  Height="71" VerticalAlignment="Top" Grid.Row="2" Grid.ColumnSpan="6" Width="352" Margin="0,0,0.444,0"/>
    </Grid>
</Window>

Хотелось бы провести рефакторинг кода. Что в нем можно улучшить?

Answer 1

1)В кнопках много логики.

Кнопки должны содержать минимум логики и по возможности дергать 1 метод, где и будет описана вся логика.

2)Почему бы не создать отдельный класс, который будет содержать 4 поля: Русский глагол, 1-форма, 2-форма,3- форма? Затем, этот класс оборачиваешь в еще 1 класс, и хранишь все в List и делаешь методы сравнения и т п. Этим ты заменишь работу с массивом в Verbs. Ты сможешь работать с конкретными полями, а не рукоблудить индексы двумерного массива.

3)Возможно, имеет смысл хранить глаголы не в TXT файле, а в XML и выполнять десериализацию.(Особенное, если будешь следовать совету 2).

Answer 2

Вот это очень плохо, т.к. путь к файлу прибит к вашему рабочему каталогу

 string[] readText = File.ReadAllLines(@"C:\Users\Рустем\Desktop\проекты\Verbs2\Verbs2\verbs.txt",
            System.Text.Encoding.GetEncoding(1251);

Да, и зачем кодировка 1251. У вас что, работа в Windows XP планируется? Все ж давно на юникод перешли!

Создайте папку в вашем проекте Assets, затем прав.клав.клик по этой папке Add->Existing Item->и укажите ваш verbs.txt установите у него свойства Build Action в Embedded Resource. А далее его можно будет прочитать так (пример из моего проекта = читаю список стран)

StrList = new List<string>();
using (Stream stream = GetType().Assembly.GetManifestResourceStream("WpfEmbeddedResource.Assets.Countries.txt"))
using (StreamReader reader = new StreamReader(stream))
{
    string input = null;
    while ((input = reader.ReadLine()) != null)
    {
        StrList.Add(input);
    }
}

WpfEmbeddedResource - это название проекта.

Answer 3

Начнем с самого основного - класса. Вместо использования двумерного массива, лучше вынести логику работы с глаголом в отдельный класс, например:

public class Verb
{
    public string Rus { get; set; }
    public string F1 { get; set; }
    public string F2 { get; set; }
    public string F3 { get; set; }
}

названия свойство можно и более содержательные придумать =)

Хранить элементы можно в обычном массиве:

    Verb[] data;
    public MainWindow()
    {
        InitializeComponent();
        data =
            File.ReadLines("data.txt")
            .Select(x => x.Split('.'))
            .Select(x => new Verb { F1 = x[0], F2 = x[1], F3 = x[2], Rus = x[3] })
            .ToArray();
    }

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

Методы GetRandom() И SetRandom избыточны. Кроме этого вы жестко задаете верхнюю границу. Можно использовать свойство .Length:

    Verb current;
    private void setNextVerb()
    {
        current = data[rnd.Next(data.Length)];
    }
    private void btnstart_Click(object sender, RoutedEventArgs e)
    {
        setNextVerb();
        btncheck.IsEnabled = true;
        counterWin = 0;
        counterLose = 0;
    }

На мой взгляд эта кнопка вообще не нужна и такой код лучше поместить, например, в конструктор.

В методе CheckVerbs вы используете конструкцию вида:

if something return true; else return false;

которая может быть сведена к

return something;  

То есть можно написать сразу так:

    //проверяем соответствуют ли введенные данные,той строке что мы выбрали
    private bool CheckVerbs(string a, string b, string c, Verb verb)
    {
        return a.Equals(verb.F1) && b.Equals(verb.F2) && c.Equals(verb.F3);
    }

Тогда ваши обработчики изменятся следующим образом:

   private void btncheck_Click(object sender, RoutedEventArgs e)
    {
        lblverb4.Content = current.F1;
        lblverb5.Content = current.F2;
        lblverb6.Content = current.F3;
        if (CheckVerbs(textverb1.Text, textverb2.Text, textverb3.Text, current))
        { ...

и

    private void btnnext_Click(object sender, RoutedEventArgs e)
    {
        setNextVerb();
        lblverb2.Content = current.Rus;

Что касается XAML разметки - она не гибкая. Попробуйте изменять размер окна и вы поймете о чем идет речь. При первой возможности напишу об этом подробнее.

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

READ ALSO
Несколько коллекций на одном Canvas

Несколько коллекций на одном Canvas

Нужно отобразить несколько коллекций на одном Canvas'еСейчас сделано так - объединил ObservableCollection's в CompositeCollection и в xaml:

460
Нужна помощь в c# [требует правки]

Нужна помощь в c# [требует правки]

По зданию надо в c# написать табулирование функции на числовом промежутке a,b с шагом p, желательно с комментариями что на каждом шаге делается,...

713
Вызвать метод из списка

Вызвать метод из списка

У меня есть список с методами

458
codeigniter одновременный запрос к двум разным БД или синхронизация таблиц в разных БД

codeigniter одновременный запрос к двум разным БД или синхронизация таблиц в разных БД

На работе по наследству досталось 6 приложений на php + Codeigniter + БД mySqlКаждая БД содержит в себе таблицы (справочники), которые повторяются в этих...

518