Раскритикуйте данный код
Код представляет из себя простое консольное приложение по рандомному обновлению курса валют. Написал я его, следуя своей программе обучения, в данном коде реализовывал 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 шт):
Весь последующий тон прошу быть прощенным ввиду изначального согласия автора на пропарку.
Насколько плохо написан код?
Отвратительно.
Какие изменения внести?
Обо всём по порядку.
версия .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);
Захордкодить не только список валют, но и курс? И то и другое, не может лежать вшитым в код. Хотя бы вынесите это всё в пару конфигурационных файлов. Один для списка валют, другой для котировок.
Пожалуй тут можно и сделать паузу. Надеюсь будет полезным.
Вкратце навскидку с точки зрения архитектуры, без придирок по мелочам.
- Мало инкапсуляции и разделения ответственности. Словарь
CryptoValute- это должно быть как минимум два отдельных класса. НапримерCryptoCurrency- собственно сама валюта, в вашем случае у валют нет других свойств кроме названия (цена - это всё-таки не внутреннее свойство валюты, а внешнее), поэтому это скорее даже должно быть перечисление, т.е.enum.CryptoPrices- хранилище цен валют, да, внутри там будет словарь, но вся логика работы с ценами такая как установка и получение цены, печать всех цен и т.п. - должна быть спрятана внутри этого класса
- Не очень понятные названия сущностей. Что такое
RestorPrice,Iterations,ExtremeValues? Из названий не очень понятно что это, они не очень стандартные. Если метод производит какое-то действие, то в названии должен быть глагол, причём какой-то типовой:Set,Save,Process,Printи т.п., а не существительное. - Не соблюдён принцип
DRY. Например,switchтут не нужен, если знать, что у словаря есть методыContainsKeyиTryGetValue. Код станет гораздо короче. Есть и другие места, где можно было бы избежать повторения кода путём некоторого рефакторинга.