Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add max/min and reduceOption methods #1167

Merged
merged 1 commit into from
Jul 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions core/src/main/scala/cats/Foldable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,90 @@ import simulacrum.typeclass
}
}

/**
* Reduce the elements of this structure down to a single value by applying
* the provided aggregation function in a left-associative manner.
*
* @return `None` if the structure is empty, otherwise the result of combining
* the cumulative left-associative result of the `f` operation over all of the
* elements.
*
* @see [[reduceRightOption]] for a right-associative alternative.
*
* @see [[Reducible#reduceLeft]] for a version that doesn't need to return an
* `Option` for structures that are guaranteed to be non-empty.
*
* Example:
* {{{
* scala> import cats.implicits._
* scala> val l = List(6, 3, 2)
* This is equivalent to (6 - 3) - 2
* scala> Foldable[List].reduceLeftOption(l)(_ - _)
* res0: Option[Int] = Some(1)
*
* scala> Foldable[List].reduceLeftOption(List.empty[Int])(_ - _)
* res1: Option[Int] = None
* }}}
*/
def reduceLeftOption[A](fa: F[A])(f: (A, A) => A): Option[A] =
reduceLeftToOption(fa)(identity)(f)

/**
* Reduce the elements of this structure down to a single value by applying
* the provided aggregation function in a right-associative manner.
*
* @return `None` if the structure is empty, otherwise the result of combining
* the cumulative right-associative result of the `f` operation over the
* `A` elements.
*
* @see [[reduceLeftOption]] for a left-associative alternative
*
* @see [[Reducible#reduceRight]] for a version that doesn't need to return an
* `Option` for structures that are guaranteed to be non-empty.
*
* Example:
* {{{
* scala> import cats.implicits._
* scala> val l = List(6, 3, 2)
* This is eqivalent to 6 - (3 - 2)
* scala> Foldable[List].reduceRightOption(l)((current, rest) => rest.map(current - _)).value
* res0: Option[Int] = Some(5)
*
* scala> Foldable[List].reduceRightOption(List.empty[Int])((current, rest) => rest.map(current - _)).value
* res1: Option[Int] = None
* }}}
*/
def reduceRightOption[A](fa: F[A])(f: (A, Eval[A]) => Eval[A]): Eval[Option[A]] =
reduceRightToOption(fa)(identity)(f)

/**
* Find the minimum `A` item in this structure according to the `Order[A]`.
*
* @return `None` if the structure is empty, otherwise the minimum element
* wrapped in a `Some`.
*
* @see [[Reducible#minimum]] for a version that doesn't need to return an
* `Option` for structures that are guaranteed to be non-empty.
*
* @see [[maximumOption]] for maximum instead of minimum.
*/
def minimumOption[A](fa: F[A])(implicit A: Order[A]): Option[A] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about minOption. Do we need such a long name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to be consistent with minimum. Is this okay considering the discussion here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally. Sorry. I'm +1 on the whole PR. My 👍 above may not have been clear about that.

reduceLeftOption(fa)(A.min)

/**
* Find the maximum `A` item in this structure according to the `Order[A]`.
*
* @return `None` if the structure is empty, otherwise the maximum element
* wrapped in a `Some`.
*
* @see [[Reducible#maximum]] for a version that doesn't need to return an
* `Option` for structures that are guaranteed to be non-empty.
*
* @see [[minimumOption]] for minimum instead of maximum.
*/
def maximumOption[A](fa: F[A])(implicit A: Order[A]): Option[A] =
reduceLeftOption(fa)(A.max)

/**
* The size of this Foldable.
*
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/scala/cats/Reducible.scala
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ import simulacrum.typeclass
val F = self
val G = Reducible[G]
}

def minimum[A](fa: F[A])(implicit A: Order[A]): A =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not .min to be closer to stdlib (and fewer chars).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this specifically to avoid collision with the std lib. If I refactor something from a NonEmptyList to a List I'd prefer to get a compile error instead of something with no type signature change but that will start throwing exceptions at runtime.

Having said that, there's a question of how much we should bend over backwards to avoid collisions with std lib names. I'm not set in stone on this if people would prefer min.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case you would have Reducible[NonEmptyList].min(myThing) how could there be a collision with the stdlib in practice? The collision could come with an implicit class that adds .min to any Reducible, and then we have anxiety, perhaps, when reading the code if we are getting the safe Reducible one or the stdlib (which would be safe in exactly the cases that the Reducible exists).

Is that what you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnynek yes that's what I mean, but note that we already have syntax enrichment for Reducible, and it's generated by Simulacrum so it will automatically inherit the name of the method on the type class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnynek do you have any further thoughts on this after my note about the syntax enrichment that currently exists?

If there's a strong preference for min then I can change it, but I do have a hesitation about potential refactoring bugs that could come out of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see...

I'm not a huge implicit syntax fan (this example, but also confusion for novices as to where methods are coming from).

But I recognize many cats users do like this, so avoiding the collision seems safest.

👍

reduceLeft(fa)(A.min)

def maximum[A](fa: F[A])(implicit A: Order[A]): A =
reduceLeft(fa)(A.max)
}

/**
Expand Down
23 changes: 23 additions & 0 deletions tests/src/test/scala/cats/tests/FoldableTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,29 @@ abstract class FoldableCheck[F[_]: Foldable](name: String)(implicit ArbFInt: Arb
fa.nonEmpty should === (iterator(fa).nonEmpty)
}
}

test("maximum/minimum") {
forAll { (fa: F[Int]) =>
val maxOpt = fa.maximumOption
val minOpt = fa.minimumOption
val list = fa.toList
val nelOpt = list.toNel
maxOpt should === (nelOpt.map(_.maximum))
maxOpt should === (nelOpt.map(_.unwrap.max))
minOpt should === (nelOpt.map(_.minimum))
minOpt should === (nelOpt.map(_.unwrap.min))
maxOpt.forall(i => fa.forall(_ <= i)) should === (true)
minOpt.forall(i => fa.forall(_ >= i)) should === (true)
}
}

test("reduceLeftOption/reduceRightOption") {
forAll { (fa: F[Int]) =>
val list = fa.toList
fa.reduceLeftOption(_ - _) should === (list.reduceLeftOption(_ - _))
fa.reduceRightOption((x, ly) => ly.map(x - _)).value should === (list.reduceRightOption(_ - _))
}
}
}

class FoldableTestsAdditional extends CatsSuite {
Expand Down