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

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

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

Страницы: 13 4 5 614 Следующая »
#45
1:50, 15 ноя. 2018

Math.Clamp не нашел, написал свой.

public class AgentAddState {

    public List<StateDefine> States; // Этот список как-то заполнен

    private static float kValueMin = 0;
    private static float kValueMax = 100;

    public void updateValue(AgentList agent, StateType type, Func<float, float> updateFunc) {
        foreach (StateDefine state in States) {
            if (state.Agent == agent && state.Type == type) {
                state.Value = clampValue(updateFunc(state.Value));
                break;
            }
        }
    }

    private float clampValue(float value) {
        return Math.Min(Math.Max(kValueMin, value), kValueMax);
    }

}

И делаем любые операции какие захотим.

agentAddState.updateValue(AgentList.Agent1, StateType.State1, value => 10);
agentAddState.updateValue(AgentList.Agent2, StateType.State2, value => value + 10);
agentAddState.updateValue(AgentList.Agent3, StateType.State3, value => value - 10);
agentAddState.updateValue(AgentList.Agent1, StateType.State4, value => value * 10);
agentAddState.updateValue(AgentList.Agent2, StateType.State5, value => value / 10);

#46
1:51, 15 ноя. 2018

tac
> Да, но ... код из Unity, хотя я пытался это утаить

Геймдев-код на C# ― это Unity с вероятностью 95%. Было бы удивительно, если не Unity.

> Unity сериализует по умолчанию только публичные списки/поля

Но не по умолчанию любые.

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

Если перечислить все виды агентов, то мы получим перечисление, которое по гайдлайнам C# рекомендуется называть существительным в единственном числе. Т.е. не Colors, не ColorList, а просто:

enum Color { Red, Green, Blue }

Если несколько значений этого перечисления потом засунуть в список, то тогда уже получим список агентов ― AgentList. В общем, добавка -List ОЧЕНЬ дезориентирует.

#47
(Правка: 1:55) 1:52, 15 ноя. 2018

tac
> и сколько таких кусков кода ты напишешь в коде?
Один.  Может быть это будет единственный вызов, в каком-то цикле. А может и потребуются и методы Set(), Get(), Add(), но только в классе, имеющим функционал, а не бестолковой обертки над массивом.
Возможно ты захочешь пофантазировать мол допустим класс в твоем коде имеет функционал, но ты сказал "рефакторинг" и написал говнокод с избыточным классом. Ответ на это очевиден для любого спеца - сносить, ессно, разве можно такое терпеть?!

#48
(Правка: 2:04) 1:59, 15 ноя. 2018

DanielSky
> А может и потребуются и методы Set(), Get(), Add(), но только в классе, имеющим
> функционал
> мол допустим класс в твоем коде имеет функционал
класс может иметь ровно такой вид, как он имел ... но при этом не быть избыточным, как только вызов его Set(), Get(), Add() вызывается не один раз извне ... и только в противном случае я согласен с
DanielSky
> избыточным классом, всякий спец тебе скажет - сносить

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

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

#49
2:12, 15 ноя. 2018

tac
> ну как бы, если речь о дублировании - то сомнительное утверждение ... дважды
> написанное выражение, это конечно не сотни раз как у patsanchik3
> var state = States.Find(f => f.Agent == argAgent && f.Type == argType);
> но тем не менее, в дальнейшим править в двух местах, а не в одном ...

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

> > blablabla.GetStateValue(agentFrom, agentTo, type).Set(30);
> (реализацию писать не буду, все вроде понятно - там будут три метода со
> сведением к одному private)

Если GetStateValue будет строго private, то ок. Но я б не стал рождать лишнюю функцию только ради сравнения двух пар значений.

#50
(Правка: 2:48) 2:24, 15 ноя. 2018

tac
> класс может иметь ровно такой вид, как он имел ... но при этом не быть
> избыточным, как только вызов его Set(), Get(), Add() вызывается не один раз
> извне
Нет. Дело не количестве обращений к нему. В чем его функционал?
Чтобы не дублировать одну строчку ты на этот класс потратил пол страницы абсолютно бестолкового кода. И гораздо важнее то, что, каждый такой пустой класс запутывает архитектуру.
Даже если у тебя обращается к массиву одинаковым образом сотня никак не связанных классов и каждый раз при этом приходится писать тонну текста, то все-равно лучше не плодить лишних классов, а спихнуть советующие методы в какой-нибудь подходящий интерфейс и вызывать, например DataHandler::AddState(map<DataDefine>& data, AgentList agent, StateType type, float value);

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

> Это кстати, тоже одна из ошибок мидл - по классу нельзя определить его
> необходимость, только по его связям ...
Эхх… каждый класс должен быть законченной сущностью сам по себе и обладать минимумом связей. Множество связей наоборот говорит не о его необходимости, а наоборот.

Ты кто такой вообще, серьезного дядьку вроде из себя строил, почему приходится кепа врубать?

#51
(Правка: 3:16) 3:14, 15 ноя. 2018
private float GetState(AgentList agent, StateType state) {
}
public void SetState(AgentList agent, StateType state, float value) {
}
public void AddToState(AgentList agent, StateType state, float value) {
  SetState(agent, state, GetState(agent, state)+value);
}
Ну и выкинуть List на мороз, оставить что то в духе: Dictionary<AgentStateKey, float> где:
private struct AgentStateKey
{
  AgentList agent;
  StateType state;
};
А если сделать, чтобы GetState возвращал ссылку, то:
public void AddToState(AgentList agent, StateType state, float value) {
  GetState(agent, state)+=value;
}
Но я не уверен, что шарпик это позволяет.
#52
(Правка: 4:31) 3:58, 15 ноя. 2018

>[b]MrShoor[/b]
> Ну и выкинуть List на мороз, оставить что то в духе: Dictionary<AgentStateKey, float>
варианты собственно решений подходят к концу .. поэтому начну, комментировать вначале более вторичные детали ..

List vs Dictionary .. действительно, часто хочется обращаться позиционно в массив, чем искать в нем /кстати запомним это на будущие, когда я буду писать про лямды/ .. и об этом написали многие .. но, это болезнь перфикциониста /позже я возьму смелость всем поставить диагнозы, о нет, не подумайте что это ad hominem, я со всем уважением, хотя и многие тут без этого .. просто так будет ярче понятно почему у меня то или иное мнение, ну это если вам интересно/ 

когда эта болезнь перфикциониста мешает? в реальном мире ... у меня тут просто наклевывается материал для статьи )

вы заметили, что число элементов в массиве крайне мало? откуда, да потому что Agent и State определяются через enum .. создавать для этого ключ для Dictionary .. ну такое себе решение - затраты времени, тем более вы разве не заметили, что цель рефакторинга другая? так зачем вам это менять? чтобы убрать поиск в цикле? создание ключа при добавлении в словарь, это лишний код, который при этом не дает абсолютно ничего .. а если как говорили уже - изменятся правила игры, и нужно будет искать по другим параметрам? переделывать ключи?

Но сам тест очень хорош, понимаешь психотип человека, который тебе отвечает.

Вообще, 80% ответов были посвещены чему угодно, но только не цели поставленной в задаче/тесте.

#53
4:02, 15 ноя. 2018

tac
> тест
Что тест?

#54
4:21, 15 ноя. 2018

MrShoor
> Что тест?
были проблемы с нетом, выше

#55
(Правка: 4:31) 4:30, 15 ноя. 2018

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

А если про перфоманс - то можно двойной массив по энумам завести (но в шарпиках нет удобного синтаксиса для этого, хахаха). List в любом случае не в дугу тут.

#56
(Правка: 4:51) 4:35, 15 ноя. 2018

Delphi смеется в лицо:

TAgentList = (
       None,
       Agent1,
       Agent2,
       Agent3
);

TStateType = (
       None,
       State1,
       State2,
       State3,
       State4,
       State5
);

TAgentAddState = class ()
private
  FStates: array [TAgentList, TStateType] of Single;
public
  procedure SetState(argAgent: TAgentList; argType: TStateType; argValue: Single);
end;

procedure TAgentAddState.SetState(argAgent: TAgentList; argType: TStateType; argValue: Single);
begin
  FStates[argAgent][argType] := argValue; //это всё, реально всё!
end;

#57
4:40, 15 ноя. 2018

MrShoor
> FStates[argAgent][argValue] := argValue;
но вы завели избыточный массив с размерами AgentCount*StateCount, в то время как не все состояния могут быть у того или иного агента ...

#58
4:41, 15 ноя. 2018

tac
> но вы завели избыточный массив с размерами AgentCount*StateCount, в то время
> как не все состояния могут быть у того или иного агента ...
Ты же сам только что говорил, что их мааало. :)

#59
(Правка: 4:49) 4:44, 15 ноя. 2018

MrShoor
> Ты же сам только что говорил, что их мааало. :)
ну тут такое мало :) 7*25 .. а всего 30 комбинаций .. ну и посчитайте, сколько у вас пустот, и какой выйгрыш баловаться со словарем или позиционным массивом, вместо поиска по 30 элементам

короткий исходный код - это не показатель его качества ))

/кстати в коде наверно описка, второй элемент по argType, а не по argValue видимо

Страницы: 13 4 5 614 Следующая »
ПрограммированиеФорумОбщее