Небольшой CodeReview?

Question

[campus1]

Ребят, изучаю C#. Делаю разные задачки.
Вот одна из них => Интервал [a, b] принимает одно из трех значений: [40, 50], [100, 90], [-300, 290].
Найти минимальное число кратное 5 и максимальное число кратное 5.

Вот как я решил =>

static void Main(string[] args)
        {
            int a = 0;
            int b = 0;
            Console.WriteLine("Enter number");
            int X = Convert.ToInt32(Console.ReadLine()); 
            int[] numbers = new int[X];
            Console.WriteLine("Choose interval:\n" + "1) -40 50;\n" + "2) -100 90;\n" + "3) -300 290;\n");
            int N = Convert.ToInt32(Console.ReadLine());
            switch (N)
            {
                case 1:
                    a = -40; b = 50;
                    break;
                case 2:
                    a = -100; b = 90;
                    break;
                case 3:
                    a = -300; b = 290;
                    break;
                default:
                    Console.Write("Error. You dont choose correct interval ");
                    break;
            }
            Random rnd = new Random();
            int rndNumb;
            for (int i = 0; i < X; i++)
            {
                rndNumb = rnd.Next(a, b);
                numbers[i] = rndNumb;  
            }
            int max = numbers[0];
            int min = numbers[0];
            for (int i = 1; i < X; i++)
            {
                if (numbers[i] > max && i % 5 == 0)
                {
                    max = numbers[i];
                }
                if (numbers[i] < min && i % 5 == 0)
                {
                    min = numbers[i];
                }
            }
            Console.Write("Maximum even element  is : {0}\n", max);
            Console.Write("Minimum even element is : {0}\n\n", min);
        }

Скажите пожалуйста нормально ли написан код или все же г**но код?
Спасибо большое!

Answer 1

[Арсений Ефремов]

Если хотите ревью кода, то вот:

1. Использование Convert.ToInt32 вместо Int32.TryParse (или обработки исключений) ведёт к непредвиденному завершению программы в некоторых случаях.
2. При попадании в блок default у вас просто выйдет управление из switch, после чего программа продолжится и её работа окажется неверной.
3. Цикл с вызовом Random.Next можно сократить до 1 строчки, убрав скобки и лишнюю переменную.
4. Можно было бы разбить такой большой Main на несколько функций. Практической пользы в данном случае не так много, но читаемость повысило бы, да и вообще.
5. Вы уверены, что там нужно делить на 5 именно счетчик цикла в условиях?

Answer 2

[Александр Кузнецов]

Что-то мне подсказывает, или я не понял условие, или ваше решение не верно.
Мне не понятно, зачем нужно:

Console.WriteLine("Enter number");
int X = Convert.ToInt32(Console.ReadLine());

Предположим, что вы тут вводите размерность интервала, тогда вот это

Random rnd = new Random();
            int rndNumb;
            for (int i = 0; i < X; i++)
            {
                rndNumb = rnd.Next(a, b);
                numbers[i] = rndNumb;  
            }

просто мусор. Тут будут повторы. Решение не будет верным.

Ну и дальше, цикл.. Я согласен, перебором можно решить, но ваш перебор не верен. Надо идти по интервалу, а не по случайным числам.

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

Comments

[campus1]

Мне не понятно, зачем нужно:

Console.WriteLine("Enter number");
int X = Convert.ToInt32(Console.ReadLine());

Это , говорит сколько чисел, должно быть в интервале.
Например вводим 10, тогда выбрали интервал допустим -100 и 90. И тогда нам рандомно генерируются 10 чисел в этом интервале.

[campus1]

Random rnd = new Random();
            int rndNumb;
            for (int i = 0; i < X; i++)
            {
                rndNumb = rnd.Next(a, b);
                numbers[i] = rndNumb;  
            }

Здесь я например хочу запушить эти рандомные значение в наш массив.

[Александр Кузнецов]

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

[campus1]

Александр Кузнецов, Я что-то запутался. У нас есть интервал. Я же должен сгенерировать случайные числа в нем. Как же по другому?

[Александр Кузнецов]

campus1, у вас есть интервал [40, 50]. В него входят числа от 40 до 50, включительно.
Это: 40,41,42,43,44,45,46,47,48,49,50. Всего 11 чисел. Чего там нужно генерировать то?