Раскритикуйте данный код

Код представляет из себя простое консольное приложение по рандомному обновлению курса валют. Написал я его, следуя своей программе обучения, в данном коде реализовывал delegate и event. Версия .NET - 4.8

Насколько плохо написан код? Какие изменения внести?

using System;
using System.Collections.Generic;

namespace CryptoBot
{
    public class Program
    {
        public static Dictionary<string, decimal> CryptoValute = new Dictionary<string, decimal>();
        public static decimal ForExtemeValue;

        private static int _count = 0;

        static CryptoPriceMonitor priceMonitor = new CryptoPriceMonitor();
        static TraderBot traderBot = new TraderBot(priceMonitor);

        public static void PrintColor(string message, ConsoleColor color)
        {
            Console.ForegroundColor = color;
            Console.WriteLine(message);
            Console.ResetColor();
        }
        static void Main(string[] args)
        {
            CryptoValute.Add("Bitcoin", 50000);
            CryptoValute.Add("DogeCoin", 3);
            CryptoValute.Add("Ethereum", 1680);
            CryptoValute.Add("USDT", 26);

            PrintColor("Choose valute: Bitcoin, DogeCoin, Ethereum, USDT", ConsoleColor.Green);
            PrintColor("Current Prices:", ConsoleColor.Green);

            foreach (var price in CryptoValute)
            {
                Console.WriteLine(price);
            }

            string Choose = Console.ReadLine();

            while (true)
            {
                switch (Choose)
                {
                    case "Bitcoin":
                        ForExtemeValue = CryptoValute["Bitcoin"];
                        Iterations("Bitcoin");
                        Choose = Console.ReadLine();
                        break;

                    case "DogeCoin":
                        ForExtemeValue = CryptoValute["DogeCoin"];
                        Iterations("DogeCoin");
                        Choose = Console.ReadLine();
                        break;

                    case "Ethereum":
                        ForExtemeValue = CryptoValute["Ethereum"];
                        Iterations("Ethereum");
                        Choose = Console.ReadLine();
                        break;

                    case "USDT":
                        ForExtemeValue = CryptoValute["USDT"];
                        Iterations("USDT");
                        Choose = Console.ReadLine();
                        break;

                    default:
                        PrintColor("There is no currency on the list.", ConsoleColor.Green);
                        Choose = Console.ReadLine();
                        break;
                }
            }

        }

        private const int IterationsCount = 1000;
        public static void Iterations(string name)
        {
            string valute;
            decimal value;

            _count = 0;
            valute = name;
            value = CryptoValute[name];

            while (_count != IterationsCount)
            {
                _count++;
                value = CryptoValute[name];
                priceMonitor.UpdatePrice(value, valute);
            }

            Console.Clear();
            PrintColor($"Here are a {IterationsCount} last iterations", ConsoleColor.Green);
            PrintColor("Choose valute: Bitcoin, DogeCoin, Ethereum, USDT", ConsoleColor.Green);
            PrintColor("Current Prices:", ConsoleColor.Green);

            foreach (var price in CryptoValute)
            {
                Console.WriteLine(price);
            }           
        }
    }
    public class CryptoPriceMonitor
    {
        public void UpdatePrice(decimal oldPrice, string name)
        {
            decimal currentPrice = oldPrice;

            PriceChanged?.Invoke(oldPrice, ExtremeValues(currentPrice), name);
        }

        private Random _random = new Random();
        private int _procent;
        private const int MaxRandomProcent = 8;
        private const int MinRandomProcent = -7;
        private decimal ExtremeValues(decimal basePrice)
        {
            decimal newPrice = basePrice;
            decimal maxValue = Program.ForExtemeValue * 100;
            decimal minValue = Program.ForExtemeValue - (Program.ForExtemeValue * 90) / 100;

            if (basePrice >= maxValue)
            {
                _procent = _random.Next(MinRandomProcent, 0);
                newPrice -= (basePrice * Math.Abs(_procent)) / 100;
                return newPrice;
            }
            if (basePrice <= minValue)
            {
                _procent = _random.Next(1, MaxRandomProcent);
                newPrice += (basePrice * Math.Abs(_procent)) / 100;
                return newPrice;
            }
            else
            {
                _procent = _random.Next(MinRandomProcent, MaxRandomProcent);
                if (_procent > 0)
                {
                    newPrice += (basePrice * Math.Abs(_procent)) / 100;
                    return newPrice;
                }
                else
                {
                    newPrice -= (basePrice * Math.Abs(_procent)) / 100;
                    return newPrice;
                }
            }
        }

        public delegate void PriceChangeHandler(decimal oldPrice, decimal newPrice, string valute);
        public event PriceChangeHandler PriceChanged;
    }

    public class TraderBot
    {
        public TraderBot(CryptoPriceMonitor monitor)
        {
            monitor.PriceChanged += OnPriceChanged;
        }

        public void OnPriceChanged(decimal oldPrice, decimal newPrice, string valute)
        {
            if (newPrice > oldPrice)
            {
                Program.PrintColor($"Цена повысилась: {oldPrice} -> {newPrice}", ConsoleColor.Green);
                RestorPrice(valute, newPrice);
            }
            else
            {
                Program.PrintColor($"Цена понизилась: {oldPrice} -> {newPrice}", ConsoleColor.Red);
                RestorPrice(valute, newPrice);
            }
        }

        public static  void RestorPrice(string valute, decimal value)
        {
            Program.CryptoValute[valute] = value;
        }
    }
}

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

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

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

Насколько плохо написан код?

Отвратительно.

Какие изменения внести?

Обо всём по порядку.

версия .NET 4.8

Что? 4.8 в 2025 году? Можно тут долго развивать дискусии, о том на сколько вам полезно прочувствовать дух истории дотнета, но присылать мне на ревью код в таком отсталом фреймворке - признак неадекватности программиста. Сложно будет найти оправдание.

namespace CryptoBot

{

Ну вот сразу дух старины. 15 лет мы мучались, тратя целых четыре символа слева в пустую, а вы назло продолжаете. namespace CryptoBot; и никаких лишних скобочек богу скобочек.

public static Dictionary<string, decimal> CryptoValute = new Dictionary<string, decimal>();

во первых, почему public кто-то снаружи будет менять этот словарь? во вторых, опять архаизмы, зачем вы после new что-то пишете кроме скобок? в третьих поле не может быть публичным, а уж тем болеее публичным и мутируемым.

public class Program

{ public static Dictionary<string, decimal> CryptoValute = new Dictionary<string, decimal>();

Кстати, вы что начинаете пихать бизнес-логику в класс Program? Он нужен только чтобы запустить приложение, ну и может остановить как-то, извольте не хранить в нём ничего, что вне этой компетенции.

private static int _count = 0;

static CryptoPriceMonitor priceMonitor = new CryptoPriceMonitor();

Т.е. вы считаете писать private или не писать, это как вам по кайфу в данную секунду времени? Тут как бы нет прям чёткой линии партии, но надо или писать всегда или не писать опять же всегда.

static CryptoPriceMonitor priceMonitor = new CryptoPriceMonitor(); static TraderBot traderBot = new TraderBot(priceMonitor);

за исключением того что new не лаконичный, это может быть в целом норм, что вы иницилизуруете классы и работаете с классами, а не интерфейсами, но имейте ввиду, что ходите по тонкому льду, Dependency inversion principle ждёт часа, для кары.

public static void PrintColor(string message, ConsoleColor color)

{

Console.ForegroundColor = color;

Console.WriteLine(message);

Console.ResetColor();

}

Отличный метод, пока не столкнётся с хоть мало-мальским распараллеливанием, и тогда ваша консоль начнёт окрашиваться, хаотично, весёло, но хаотично.

CryptoValute.Add("Bitcoin", 50000);

CryptoValute.Add("DogeCoin", 3);

CryptoValute.Add("Ethereum", 1680);

CryptoValute.Add("USDT", 26);

Захордкодить не только список валют, но и курс? И то и другое, не может лежать вшитым в код. Хотя бы вынесите это всё в пару конфигурационных файлов. Один для списка валют, другой для котировок.

Пожалуй тут можно и сделать паузу. Надеюсь будет полезным.

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

Вкратце навскидку с точки зрения архитектуры, без придирок по мелочам.

  • Мало инкапсуляции и разделения ответственности. Словарь CryptoValute - это должно быть как минимум два отдельных класса. Например
    • CryptoCurrency - собственно сама валюта, в вашем случае у валют нет других свойств кроме названия (цена - это всё-таки не внутреннее свойство валюты, а внешнее), поэтому это скорее даже должно быть перечисление, т.е. enum.
    • CryptoPrices - хранилище цен валют, да, внутри там будет словарь, но вся логика работы с ценами такая как установка и получение цены, печать всех цен и т.п. - должна быть спрятана внутри этого класса
  • Не очень понятные названия сущностей. Что такое RestorPrice, Iterations, ExtremeValues? Из названий не очень понятно что это, они не очень стандартные. Если метод производит какое-то действие, то в названии должен быть глагол, причём какой-то типовой: Set, Save, Process, Print и т.п., а не существительное.
  • Не соблюдён принцип DRY. Например, switch тут не нужен, если знать, что у словаря есть методы ContainsKey и TryGetValue. Код станет гораздо короче. Есть и другие места, где можно было бы избежать повторения кода путём некоторого рефакторинга.
→ Ссылка