Корректировка кода консольного калькулятора на C#

Question

У меня есть код консольного калькулятора, написан на C#, сделан в целях изучения. Не давно перешел на этот язык с Java, кружечка с кофе мне не подходила под интересы, программирую 2 года, но знаете, как-то все странно получалось, я скорее не старался понять язык, а изучил основы и дальше все на ходу доучивал, если кратко занимался разработкой чисто для релакса (хз как это, но это именно так) и неважно. В связи с тем, что я решил перейти на другой язык, мне показалось интересным начать именно изучать язык и понимать, как и что работает, а не писать говнокод. Я прошу у вас, программистов со стажем, посмотреть мой код и указать на ошибки или не логические действия, может где-то какую-то строку нужно убрать и заменить другой и т.п. В общем прошу, если не составит труда, направить меня на мои ошибки. Вот код:

namespace ConsoleApp1
{
    class Program
    {

        private static int A;
        private static int B;

        static void Main(string[] args)
        {
            StartProgramm();
            Close();
        }

        private static void StartProgramm()//полагает начало программы
        {
            MathGetNumber();
            MathOperation();
        }

        private static void MathGetNumber()//получаем число
        {
            string AT;
            string BT;

            Console.WriteLine("Первое число");
            AT = Console.ReadLine();
            Console.WriteLine("Второе число");
            BT = Console.ReadLine();

            try
            {
                A = Convert.ToInt32(AT);
                B = Convert.ToInt32(BT);
            }
            catch (FormatException)
            {
                Console.WriteLine("Не шали, вводи только числа!");
                StartProgramm();
            }

            for (byte i = 0; i < 4; i++)//это я решил сделать отступ в несколько строк
            {
                Console.WriteLine();
            }
        }

        private static void MathOperation()//производим операцию с числом
        {
            byte Operate;
            string Operation;

            if (A == 0 && B == 0)
            {
                Console.WriteLine("Числа имеют значение 0, продолжать работу программы нет смысла, завершение");
            }
            else
            {
                const byte Longe = 4;
                byte OperNum = 1;
                char[] NumUs = new char[Longe] { '+', '-', '*', ':' };
                Console.WriteLine("Числа имеют значение > или < 0, выберите желаемую операцию над числом");

                for (byte i = 0; i < Longe; i++)
                {
                    Console.WriteLine(OperNum + ") " + NumUs[i]);
                    OperNum++;
                }

                Operation = Console.ReadLine();

                try
                {
                    Operate = Convert.ToByte(Operation);

                    if (Operate <= 4)
                    {

                        switch (Operate)
                        {
                            case 1:
                                Console.WriteLine("Ваше число = " + (A + B));
                                break;
                            case 2:
                                Console.WriteLine("Ваше число = " + (A - B));
                                break;
                            case 3:
                                Console.WriteLine("Ваше число = " + (A * B));
                                break;
                            case 4:
                                Console.WriteLine("Ваше число = " + (A / B));
                                break;
                        }
                    }
                    else
                    {
                        Console.WriteLine("Хватит шалить, вводи число от 1 до 4 включительно");
                        MathOperation();
                    }
                }
                catch (FormatException)
                {
                    Console.WriteLine("Хватит шалить, вводи число от 1 до 4 включительно");
                    MathOperation();
                }
            }
        }

        private static void ReturnProgramm()//повторение работы программы с самого начала, на случай если пользователь захочет еще раз посчитать число 
        {
                StartProgramm();
                Close();
            }
        }

        private static void Close()//завершение работы программы
        {
            string answer;

            Console.WriteLine("Close program? y/n");
            answer = Console.ReadLine();

            if (answer == "y")
            {
                return;
            }
            else if (answer == "n")
            {
                ReturnProgramm();
            }
            else
            {
                Close();
            }
        }

    }
}

Недавно было что то подобное, там полноценно описали какие ошибки, как стоит поступать и так далее. Советую почитать. — EvgeniyZ, May 22 '19 at 14:08
спасибо, ребят, обязательно все почитаю — Dya, May 22 '19 at 14:24
Попробуйте статический анализ. Откройте в Visual Studio свойства проекта. Вкладка Code Analysis. Чекните Enable Code Analysis on Build. Выберите в комбобоксе набор правил (ставьте сразу Microsoft All Rules). Скомпилируйте проект. Будет большущий список ошибок-предупреждений. Изучайте ;) — Alexander Petrov, May 23 '19 at 12:39

score 4 · Accepted Answer · answered May 23 '19 at 07:54

Вообще каждый программист находит для себя свой стиль. Важно лишь соблюдать базовые принципы, заложенные той или иной технологией.

Мой ответ будет основан исключительно на собственном опыте и может не совпадать с мнением других.

Первое, что сразу бросилось в глаза - это методы без параметров и возвращаемых значений. Да, есть методы, которые могут быть без параметров и не возвращать ничего (собственно, ваши методы StartProgram() и Close()). Но вот лично мне хотелось бы, чтобы метод MathGetNumber() возвращал значение, а не присваивал его какой-то глобальной переменной. Желательно, чтобы метод получал необходимые значения, проводил с ними определенные манипуляции и в случае необходимости возвращал значение. Если хотите все же оставить метод таким, то тогда лучше было бы его назвать, например, так: MathReadAB(). Старайтесь, чтобы ваш метод делал ровно то, о чем говорит его название.

Второе: в C# в классах private-поля принято именовать с маленькой буквы:

private static int a;

а еще некоторые программисты к private-полям добавляют приставку с нижним подчеркиванием (это уже дело вкуса; при стандартных настройках Visual Studio подсказками предлагает именно такой вариант именования):

private static int _a;

Для C#, как и для любого другого ЯП, есть соглашение по именованию членов, переменных, функций... и соглашение по написанию кода.

Третье: не пытайтесь сэкономить на памяти, объявляя вместо int переменную типа byte. Это, может быть, и хорошо, однако излишняя оптимизация часто не оправдана. В данном случае, мне кажется, она не оправдана. К тому же, если вы проявите невнимательность, то тип byte может переполниться без вашего ведома, что может привести к непредсказуемым результатам работы программы. В мировой практике для итераторов цикла и любых других счетчиков принято брать тип int.

Четвертое: В методе MathOperation() можно избавиться от лишних фигурных скобок, заменив

if (A == 0 && B == 0)
{
    Console.WriteLine("Числа имеют значение 0, продолжать работу программы нет смысла, завершение");
}
else
{
    //else-code
}

на

if (A == 0 && B == 0)
{
    Console.WriteLine("Числа имеют значение 0, продолжать работу программы нет смысла, завершение");
    return;
}

//else-code

Это довольно частая и хорошая практика, иначе если у вас в коде будет много проверок, будет много вложенных else и фигурных скобок, что ухудшает читаемость кода.

Пятое: Это чисто логическая ошибка. Если у вас A=1 и B=0, то программа продолжит свою работу, что приведет к DivideByZeroException. Нужно переделать условие if (B == 0). Этого будет достаточно.

Шестое: Вместо конструкции

if (Operate <= 4)
{
    switch (Operate)
    {
        case 1:
            Console.WriteLine("Ваше число = " + (A + B));
            break;
        case 2:
            Console.WriteLine("Ваше число = " + (A - B));
            break;
        case 3:
            Console.WriteLine("Ваше число = " + (A* B));
            break;
        case 4:
            Console.WriteLine("Ваше число = " + (A / B));
            break;
    }
}
else
{
    Console.WriteLine("Хватит шалить, вводи число от 1 до 4 включительно");
    MathOperation();
}

лучше воспользоваться default:

    switch (Operate)
    {
        case 1:
            Console.WriteLine("Ваше число = " + (A + B));
            break;
        case 2:
            Console.WriteLine("Ваше число = " + (A - B));
            break;
        case 3:
            Console.WriteLine("Ваше число = " + (A* B));
            break;
        case 4:
            Console.WriteLine("Ваше число = " + (A / B));
            break;
        default:
            Console.WriteLine("Хватит шалить, вводи число от 1 до 4 включительно");
            MathOperation();
            break;
    }

В остальном все хорошо. Только Ваш стиль с объявлением переменных в начале метода - для меня непривычен, но он имеет место быть. Постарался осветить все проблемные места, что увидел.

студия не предлагает подчеркивание для приватных переменных по умолчанию. — Grundy, May 23 '19 at 09:09
По поводу четвёртого пункта. Во многих руководствах считается, что методы (и циклы) должны иметь одну точку выхода, поэтому нельзя использовать return таким способом. Более того, во многих языках программирования return вообще отсутствует, именно по этой причине. Но лично я его использую. Просто довожу до общего сведения. — Alexander Petrov, May 23 '19 at 12:30
@Grundy В Community-версии действительно по умолчанию нет такого поведения. У меня на 2015 Professional вроде бы по умолчанию стояли эти подсказки. Я еще приду домой и проверю это — Andrei Khotko, May 23 '19 at 12:52
@AlexanderPetrov соглашусь с вами. Очень часто встречал такие методики и сам стараюсь их придерживаться, но вот в подобных случаях хочется избежать лишних объявлений переменных и лишней вложенности. — Andrei Khotko, May 23 '19 at 13:01
Благодарю, все четко и без воды. Постараюсь учесть все выше сказанное) — Dya, May 23 '19 at 16:04

score 3 · Answer 2 · answered May 24 '19 at 09:00

В дополнение:

A = Convert.ToInt32(AT);
B = Convert.ToInt32(BT);

При использовании методов класса Convert единственный способ обработки неверного ввода - исключения. Генерация исключений является несколько тяжелой операцией, поэтому лучше использовать метод TryParse.

Console.WriteLine("Числа имеют значение 0, продолжать работу программы нет смысла, завершение");

Такая проверка не имеет смысла, арифметические операции с нолями вполне допустимы (кроме деления). 0+0 должно выдавать 0, а не ошибку.

char[] NumUs = new char[Longe] { '+', '-', '*', ':' };
//...
for (byte i = 0; i < Longe; i++)

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

Console.WriteLine("Хватит шалить, вводи число от 1 до 4 включительно");

Вы предлагаете пользователю ввести число, а затем преобразуете его в символ. Почему бы сразу не вводить символ? Это намного проще и удобнее.

Console.WriteLine("Close program? y/n");
answer = Console.ReadLine();

Так как вы просите пользователя ввести один из символов, оба из которых требуют одиночного нажатия клавиши, можно сделать его ввод чуть более удобным с использованием Console.ReadKey.

private static void StartProgramm()//полагает начало программы
{
    MathGetNumber();
    MathOperation();
}

Этот код привносит в программу "повторную входимость": StartProgramm вызывает MathGetNumber, который, в свою очередь, вызывает StartProgramm. Это приводит к тому, что стек вызовов в ходе работы программы будет постепенно расти. Для реализации повторяемости лучше использовать циклы, а не повторно входимые методы.

Мой вариант улучшения с учетом этих замечаний:

using System;
using System.Text;

namespace ConsoleApp1
{
    class Program
    {
        private static int A;
        private static int B;

        static void Main(string[] args)
        {
            bool success;
            while (true)
            {
                while (true)
                {
                    success = MathGetNumber();
                    if (success) break;
                }

                while (true)
                {
                    success = MathOperation();
                    if (success) break;
                }                

                if (Close()) break;                 
            }
        }       

        private static bool MathGetNumber()//получаем число
        {
            string AT;
            string BT;

            Console.WriteLine("Первое число");
            AT = Console.ReadLine();
            Console.WriteLine("Второе число");
            BT = Console.ReadLine();

            if (Int32.TryParse(AT, out A) == false || Int32.TryParse(BT, out B) == false)
            {
                Console.WriteLine("Не шали, вводи только числа!");
                return false;
            }

            for (byte i = 0; i < 4; i++)//это я решил сделать отступ в несколько строк
            {
                Console.WriteLine();
            }
            return true;
        }

        private static bool MathOperation()//производим операцию с числом
        {                       

            char[] NumUs = new char[] { '+', '-', '*', ':' };

            Console.WriteLine("Введите один из знаков операций: ");
            for (byte i = 0; i < NumUs.Length; i++)
            {
                Console.Write(NumUs[i]+" ");                
            }
            Console.WriteLine();

            string sign = Console.ReadLine();

            switch (sign)
            {
                case "+":
                    Console.WriteLine("Ваше число = " + (A + B));
                    break;
                case "-":
                    Console.WriteLine("Ваше число = " + (A - B));
                    break;
                case "*":
                    Console.WriteLine("Ваше число = " + (A * B));
                    break;
                case ":":
                    if (B == 0) Console.WriteLine("Делить на ноль нельзя");
                    else Console.WriteLine("Ваше число = " + (A / B));
                    break;
                default:
                    Console.WriteLine("Не шали, введи один из допустимых знаков операции!");
                    return false;
            }
            return true;

        }                

        private static bool Close()//завершение работы программы
        {
            char answer;
            Console.WriteLine("Close program? y/n");

            while (true)
            {
                answer = Console.ReadKey(true).KeyChar;

                if (answer == 'y')
                {
                    return true;
                }
                else if (answer == 'n')
                {
                    return false;
                }                
            }            
        }

    }
}

Корректировка кода консольного калькулятора на C#

2 Answers2