I wrote a program that randomly chooses a verb in Russian from a text document and shows it to us. The task of the user is that he should enter the same verb in 1,2,3 forms of the English language into text boxes. Actually the code itself:

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; } } }

program interface

XAML code:

 <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>

I would like to refactor the code. What can be improved in it?

    3 answers 3

    Let's start with the most basic - class. Instead of using a two-dimensional array, it is better to put the logic of working with the verb into a separate class, for example:

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

    property names can be thought up and more meaningful =)

    You can store items in a regular array:

      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(); }

    Note the use of a relative path instead of an absolute one. Instead of parsing the string, you can use deserialization in a convenient format.

    The GetRandom() and SetRandom redundant. In addition, you rigidly set the upper limit. You can use the property .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; }

    In my opinion, this button is not needed at all and it is better to place such code, for example, in the constructor.

    In the CheckVerbs method CheckVerbs you use the following construction:

     if something return true; else return false;

    which can be reduced to

     return something;

    That is, you can write right away like this:

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

    Then your handlers will change as follows:

      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)) { ...

    and

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

    As for XAML markup, it is not flexible. Try to resize the window and you will understand what is being said. At the first opportunity I will write about it in more detail.

    The best solution is to rewrite the code through MVVM . I do not want to give this option here, as it will be much better if you do it yourself.

    • Thank you for all the details painted. Without rewriting code through MVVM, is this all that can be improved? - Draktharon
    • @Draktharon sorry for not answering for a long time. It was not possible. There is nothing difficult about switching to MVVM . If you want we can do it in stages. For faster and more convenient communication I suggest to go to chat - user227049

    1) The buttons have a lot of logic.

    Buttons should contain a minimum of logic and, if possible, pull 1 method, where all the logic will be described.

    2) Why not create a separate class that will contain 4 fields: Russian verb, 1-form, 2-form, 3-form? Then, you wrap this class in 1 more class, and you keep everything in the List and you make comparison methods, and so on. This will replace the work with the array in Verbs. You will be able to work with specific fields, and not to masturbate the indices of a two-dimensional array.

    3) It may make sense to store the verbs not in a TXT file, but in XML and perform de-serialization. (Especially if you follow the advice 2).

      This is very bad, because file path is nailed to your working directory

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

      Yes, and why is the encoding 1251. Do you plan to work in Windows XP? All for a long time passed to Unicode!

      Create a folder in your Assets project, then rights.clav.click on this folder Add-> Existing Item-> and specify your verbs.txt set its Build Action properties in the Embedded Resource. And then it can be read as follows (an example from my project = I am reading the list of countries)

       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 is the name of the project.