Войти
ПрограммированиеФорумОбщее

Тест на рефакторинг (C#) (3 стр)

Advanced: Тема повышенной сложности или важная.

Страницы: 1 2 3 4 514 Следующая »
#30
(Правка: 0:58) 0:55, 15 ноя. 2018

tac
> ну, это уже можно спрятать внутри - например, вернуть не null, а объект
> StateDefine с State = null
То есть вместо проверки вводим новую сущность. Такое себе решение.

Лучше уж так:

GetState(agent, state)?.Set(10);

Если версия дотнета позволяет. В Unity до сих пор стабильный только 3.0 :(

#31
0:58, 15 ноя. 2018

alexzzzz

+ Показать
#32
(Правка: 1:03) 1:01, 15 ноя. 2018

alexey.ch
> Такое себе решение.
ой, зато "?" верх синтаксического сахара, нет уж ... это делает код малочитаемым, а главное в данном случае, заставляет вызывающего думать о том, что может быть null, хотя это дело внутренние (и думать всегда, когда работаешь с этим классом) ... т.к. классическое нарушение "раскрытия деталей класса"

#33
1:04, 15 ноя. 2018

DanielSky
> AgentAddState избыточный класс, его надо снести. А вместо него достаточно мапы,
> к которой ты будешь обращаться в одной строке. А ограничения при присваивании -
> это проблема класса, которому ты присваиваешь значение.
кодом напиши :)

#34
1:07, 15 ноя. 2018

patsanchik3
> поля, надо заменить на свойства и Clamp применять в сетере/мутаторе, всё
> остальное через Find решается одной строкой
кодом напиши, а то вы так будете долго крутится как ужи на сковородке, когда потом начнешь прижимать :)

#35
(Правка: 1:08) 1:07, 15 ноя. 2018

tac
> ой, зато "?" верх синтаксического сахара, нет уж ... это делает код малочитаемым
Это спорно. На мой взгляд отличная конструкция, которой мне не хватает в Unity.

> заставляет вызывающего думать о том, что может быть null
А в противном случае вызывающему возвращается живой объект и если он не догадается проверить костыль "State", то не поймет, что перед ним фейк.

#36
1:21, 15 ноя. 2018

alexzzzz
> Смущают несколько вещей:
в целом со списком согласен, хороший список вопросов на уточнение ... дам пояснения, которые не давал для частного "моего" случая ... но в общем случае, повторюсь, со списком согласен

> 1. Название класса должно обозначать какую-то сущность, быть существительным.
> AgentAddState непонятно что обозначает.
Дополнительные свойства агента :)

> 2. Параметр argAgent вроде бы обозначает одного «агента», но его тип называется
> AgentList, будто это коллекция.
Если перечислить все виды агентов, то мы получим их список ) в то время как элемент их этого списка будет определять одного агента

> 3. Само слово arg в названиях параметров лишнее. Хотя, может это стиль такой.
стиль, стиль ... и вполне не плохой

> 4. Базовый тип у перечислений AgentList и StateType можно было бы сменить с
> умолчательного int на byte, если вместимости байта хватает для всех значений.
> Чего зря раздувать коллекцию...
ой, ну чего нам мелочится

> 5. Слово state/состояние, имхо, плохо подходит для обозначения чего-то от 0 до
> 100. Я бы назвал состояния агента не состояниями, а атрибутами или параметрами
> или ещё как-то. А состояние агента ― по смыслу это совокупность текущих
> значений всех его атрибутов.
ну в классическом смысле ООП слэнга согласен, так и есть ... но тут под состоянием понимается именно одна его характеристика, и в общем случае, на "бизнес языке" она может называться одним из состояний объекта ... уж очень перегруженное слово значениями


> 6. Магические числа в коде: 0 и 100. Ноль ― ещё ладно, а 100 неплохо бы
> заменить на константу MAX_STATE_VALUE или типа того.
ну, ок ... мелочи

> 7. Если в коллекции не нашлось подходящего элемента и мы об этом умолчали, это
> нормально?
хорошие поведение, игнорировать - не явно это подразумевал, но кидать исключение плохой вариант, т.к. форма хранения List как раз подразумевает, что не все агенты с их не всеми свойствами будут заполнены.

> 8. Раз существует некий класс, который хранит значения неких характеристик
> неких агентов, и у этого класса есть прям публичный метод, который шарится по
> этим характеристикам и чего-то в них меняет, то наверное способ хранения этих
> данных (в List<T> или ещё в чём) ― это детали реализации этого класса и никому
> другому их знать не стоит. Поэтому список стоит сделать приватным, чтобы работа
> с данными велась только через публичные методы и при желании/надобности
> реализацию можно было изменить и остальной код ничего бы не заметил.

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

#37
(Правка: 1:25) 1:24, 15 ноя. 2018

tac
> А те же грабли
Да похер.

+ Показать
#38
(Правка: 1:25) 1:24, 15 ноя. 2018

tac
> кодом напиши, а то вы так будете долго крутится как ужи на сковородке, когда
> потом начнешь прижимать :)

public class StateDefine
    {
        public readonly AgentList Agent;
        public readonly StateType Type;

        public float Value
        {
            get => _value;
            set => _value = value.Clamp(LimitValueMin, LimitValueMax);
        }

        private float _value;
        private const float LimitValueMax = 100f;
        private const float LimitValueMin = 0f;

        public StateDefine(AgentList agent, StateType type)
        {
            Agent = agent;
            Type = type;
        }
    }

/* бла бла
var state = States.Find(f => f.Agent == argAgent && f.Type == argType);
if (state != null)
  state.Value += addValue;
#39
1:24, 15 ноя. 2018

alexey.ch
> он не догадается проверить костыль "State", то не поймет, что перед ним фейк
ну это если он начнет вызывать только GetState, а так это будет делать не он, а Set/Add ... впрочем, вариант с закрытым GetState может тем и лучше

#40
(Правка: 1:36) 1:34, 15 ноя. 2018

Я на шарпе очень давно не кодил, там что до сих пор нельзя

if (var state = States.Find(f => f.Agent == argAgent && f.Type == argType)) 
    state.Value += addValue;
чтоли?

#41
(Правка: 1:36) 1:35, 15 ноя. 2018

patsanchik3
> var state = States.Find(f => f.Agent == argAgent && f.Type == argType);
> if (state != null)
> state.Value += addValue;


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

var state = States.Find(f => f.Agent == КонкретныйAgent && f.Type == КонкретныйType);
if (state != null)
  ... // то что ему надо

и ты получишь в коде гавно код, просто потому что устранил класс

#42
1:42, 15 ноя. 2018

tac
> и ты получишь в коде гавно код, просто потому что устранил класс
идеальный код - это его отсутсвие, (принцип бритвы оккама)
если тебе нужен элемент коллекции - ты его берешь и используешь, а плодят классы на каждый чих тока такие как ТС - ну - зато всегда есть что порефакторить, тратя своё время и деньги заказчика :)

#43
(Правка: 1:49) 1:46, 15 ноя. 2018

alexzzzz
> Да похер

ну как бы, если речь о дублировании - то сомнительное утверждение ... дважды написанное выражение, это конечно не сотни раз как у patsanchik3

var state = States.Find(f => f.Agent == argAgent && f.Type == argType);

но тем не менее, в дальнейшим править в двух местах, а не в одном ...

alexzzzz
> blablabla.GetStateValue(agent, type).Set(30);
> Это будут кишки наружу. При необходимости изменить способ хранения данных
> придётся или везде код править или сочинять костыли для совместимости с
> существующим работающим кодом.

Спорно, тут наружу только параметры функции, на основе которых сейчас нужно искать. Ну да, если например расширим до состояния не агента, а между агентами, то придется указывать двух агентов и тип, но тогда мы сделаем полиморвизм вида
> blablabla.GetStateValue(agentFrom, agentTo, type).Set(30);
(реализацию писать не буду, все вроде понятно - там будут три метода со сведением к одному private)
а прошлый вариант так и останется, для состояний одного агента - и никаких костылей ... в любом случае, это не кишки наружу, а всего сигнатура текущей функциональности

#44
(Правка: 1:48) 1:47, 15 ноя. 2018
patsanchik3
> идеальный код - это его отсутсвие
а ну ок :) с вами мы закончили )) мы вам позвоним
Страницы: 1 2 3 4 514 Следующая »
ПрограммированиеФорумОбщее