Replace indexed skill access with skills.OfType<...>().Single()
#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
There was a problem hiding this comment.
Is that to say they can't distinguish by based on a property for some reason?
There was a problem hiding this comment.
Right now to display the skill name it just takes the type name. I could add an exception for aim, but I feel like it's an ugly way cos for example if someone works on aim revamping they'll need to remove it, or if someone add another multipass skill it'll need another exception and so on, and that'd need to happen not only in tools but in diffcalc itself. Creating a separate class seems like a more elegant solution to me
There was a problem hiding this comment.
Hmm. In general I'd expect that to be handled by a Name property or similar, but I suppose this is fine for now.
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 |
this change is very good when you have something like |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Ignore the above suggestion - I'll be working on it as a separate solo effort. |
|
!difficulty |
|
!difficulty |
|
!difficulty |
|
!difficulty |
|
@stanriders please double check merge conflict resolution in 9818f90 !difficulty |
|
I can't with the merge conflicts here........ |
I'm planning to resolve and merge it after we're done actively finishing up and merging reworks, to not interfere with that |
stanriders
changed the title
skills.First(s is ...)skills.OfType<...>().Single()
Something that shouldve been done long ago