Min/max functionality - Githubissues

TabletopAssistant / DiceKit

Swift framework for expressing and evaluating dice notation (e.g., d20, 4d6+4, 3d8×10+2), which is commonly used in tabletop role-playing games.

Apache License 2.0

14 stars 1 forks source link

Min/max functionality #70

Closed LoganJohnson closed 8 years ago

LoganJohnson commented 9 years ago

Right now the implementation is super simple, which isn't necessarily a bad thing.

Ideas on tests would be awesome too.

Closes #64

brentleyjones commented 9 years ago

/Users/vagrant/git/DiceKit/FrequencyDistribution.swift:241:42: error: cannot convert return expression of type 'OutcomeType' to expected return type 'Int'
        return self.orderedOutcomes.last ?? 0
               ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

Another reason we can't change it to 0. Just return as is :smile:

LoganJohnson commented 9 years ago

Sourcetree's “amend last commit” is funky. I’ll be squashing this down after it's approved.

brentleyjones commented 9 years ago

This looks good, for FrequencyDistribution. Do you plan on one PR for this, ProbabilityMass, ExpressionType, and ExpressionResultType?

If so, FrequencyDistribution and ProbabilityMass would be one commit and the other commit would be the expression.

LoganJohnson commented 9 years ago

Yeah adding ProbabilityMass to this PR makes sense. I'll do that quick now.

LoganJohnson commented 9 years ago

I have the initial groundwork for expressions laid out in the latest commit, but I'm running into trouble fully implementing it. Determining the value within the MinimizationExpressionResult means that you would need to pass through a probabilityMass to be able to pull a minimumOutcome() from it. I'll be working on this later tonight so feedback tonight would be awesome.

brentleyjones commented 9 years ago

I took the day off yesterday. I'll try to look at this today, but it might have to be after D&D Tuesday (so, Wednesday).

brentleyjones commented 9 years ago

But, without looking at it, I think it would work this way:

For probabilityMass, which lives in Expression, yes, get the min/max from the probabilityMass of the base expression. Then just make a new ProbabilityMass with that constant. Done.
For result, you just pass in the value from the expression (along with the result object for base). The reason for this is because we still need to show what it would have been, but we don't care, and for value we would simply return the value given to us in initialization.

brentleyjones commented 8 years ago

Almost there!

Can you add a section to the CHANGELOG? Also, adding something to the playground would be swell as well.

LoganJohnson commented 8 years ago

Resolving these conflicts tonight. Got too late last night.

brentleyjones commented 8 years ago

It will be nice to have this.

Can you add something to the CHANGELOG?

Also, do you plan on making a new page for "operations", since min/max will be the first one? (You don't have to, someone else can as well)

LoganJohnson commented 8 years ago

Alright I think we are good here? I'll squash it after a review.

brentleyjones commented 8 years ago

@LoganJohnson cleanup the issues pointed out and then squash. Then when the build passes CI you can merge it.