Replace indexed skill access with skills.First(s is ...)
#30034
Conversation
There was a problem hiding this comment.
Since this is a code quality diffcalc change rather than just a diffcalc change, I'll bite.
I guess this is an improvement but I'm not sure this goes far enough? As in, if we're going to such lengths, then why not a custom thin wrapper data structure, e.g. your basic container?
public class SkillContainer
{
private Dictionary<Type, object> skills;
public Set<T>(T skill) => skills[typeof(T)] = skill;
public Get<T>(T skill) => (T)skills[typeof(T)]!;
}or something.
Heck, if it didn't fill incredibly gross to reuse it in this context, that's what DependencyContainer is.
| @@ -40,7 +41,7 @@ protected override DifficultyAttributes CreateDifficultyAttributes(IBeatmap beat | |||
|
|
|||
| CatchDifficultyAttributes attributes = new CatchDifficultyAttributes | |||
| { | |||
| StarRating = Math.Sqrt(skills[0].DifficultyValue()) * difficulty_multiplier, | |||
| StarRating = Math.Sqrt(skills.First(s => s is Movement).DifficultyValue()) * difficulty_multiplier, | |||
There was a problem hiding this comment.
These should really say .Single().
| /// <summary> | ||
| /// Represents the Aim skill that does not include sliders length in it's calculation | ||
| /// </summary> | ||
| public class AimWithoutSliders : Aim |
There was a problem hiding this comment.
And this is... kinda weird? Why bother?
Why not expose WithSliders as public readonly on the skill, and then find it where you need it based on that flag alone? As in
skills.OfType<Aim>().SingleOrDefault(aim => aim.WithSliders)or something.
There was a problem hiding this comment.
It's mostly for the tools, to be able to distinguish between two aim skills
I'm not sure if that's really necessery honestly, since all this tries to achieve is save someone from inevitabely shooting themselves in a foot by messing up indices |
Something that shouldve been done long ago