[Issue-35] Added orEmpty and orZero functions for Option #44

sohum2002 · 2017-10-18T08:23:30Z

Implemented orEmpty helper for Option[String]
Implemented orEmpty helper for Option[Iterable[Empty]]
Implemented orEmpty helper for Option[Numeric]
Created tests accordingly to test all three functions

dgouyette · 2017-10-18T09:14:58Z

Thanks for your contribution @sohum2002 . I'll check it ASAP

dgouyette

I thing we should be inspired by cats version which use monoid. Simple and functionnal.
WDYT ?

def orEmpty(implicit A: Monoid[A]): A = oa.getOrElse(A.empty)

dgouyette · 2017-10-18T14:48:03Z

shared/src/test/scala/OptionHelperSpec.scala

+    optInt.orZero shouldEqual 0
+  }
+  "A none element of Option[Float]" should "return 0f" in {
+    val optInt: Option[Int] = None


Copy/paste ? it's the wrong type

dgouyette · 2017-10-18T14:48:21Z

shared/src/test/scala/OptionHelperSpec.scala

+    optInt.orZero shouldEqual 0f
+  }
+  "A none element of Option[Double]" should "return 0d" in {
+    val optInt: Option[Int] = None


Copy/paste ? it's the wrong type

dgouyette · 2017-10-18T14:48:51Z

shared/src/test/scala/OptionHelperSpec.scala

+    optString.orEmpty shouldEqual ""
+  }
+  "An none element of Option[Iterable]" should "return empty iterable" in {
+    val optList: Option[List[Any]] = None


Any ? Why it's not T ? It must works with all the type

Fixed that in the latest commit :)

It does not seem to be fixed :
https://github.com/sohum2002/hamsters/blob/issue-35/shared/src/test/scala/OptionHelperSpec.scala#L13

sohum2002 · 2017-10-18T18:12:28Z

@dgouyette I've made some changes as per your comments, thanks for that! Regarding Monoid's, I'm not too sure how they are used in Scala, will look into it.

EDIT: I did a quick search on Monoids and don't think it's suitable to implement this for Monoids.

loicdescotte · 2017-10-21T07:20:45Z

shared/src/main/scala/io/github/hamsters/OptionHelper.scala

@@ -0,0 +1,22 @@
+package io.github.hamsters
+
+object OptionHelper {


Would be better to avoid *Helper name for objects :)
The name should explain what the object is doing.

sohum2002 · 2017-10-21T10:08:05Z

Thank you @loicdescotte for your comments to help improve my pull request. I made the changes you recommended, so when you're free please take a look! Looking forward to it :)

dgouyette · 2017-10-21T13:20:03Z

@sohum2002 Monoids provide a zero element.
So i think , that would help http://eed3si9n.com/learning-scalaz/Monoid.html because orEmpty give us a zero element for a given type

loicdescotte · 2017-10-22T07:22:11Z

shared/src/main/scala/io/github/hamsters/OptionDefaults.scala

@@ -1,6 +1,6 @@
 package io.github.hamsters

-object OptionHelper {
+object OptionDefaults {


As it handles empty values, something like EmptyOptionValues would be better IMO.

loicdescotte · 2017-10-22T07:22:36Z

shared/src/main/scala/io/github/hamsters/OptionDefaults.scala

@@ -1,6 +1,6 @@
 package io.github.hamsters

-object OptionHelper {
+object OptionDefaults {
  implicit class StringHelper(optStr: Option[String]){


Other Helper should be renamed too :)

sohum2002 · 2017-10-22T09:07:38Z

@dgouyette Monoids look like an interesting implementation for hamsters project. However, I think this should be separated out into another issue/feature request? Let me know what you think!

loicdescotte · 2017-10-22T09:46:49Z

shared/src/main/scala/io/github/hamsters/EmptyOptionValues.scala

+package io.github.hamsters
+
+object EmptyOptionValues {
+  implicit class StringOptionValues(optStr: Option[String]){


StringOptionValues should not be plural as there is a single default value for empty Option[String].
Also it should reflect the fact that it's a value for an empty option.
(same comment for other implicit classes)

What do you think of this name: EmptyStringOptionValue

I suggest the creation of trait that represents default value :
trait DefaultValue[+A]{ def get : A }
and put typed implementation in object :
`object DefaultValue{

implicit def intDefaultValue = new DefaultValue[Int] {
override def get = 0
}

implicit def stringDefaultValue = new DefaultValue[String] {
override def get = ""
}
}`

So, in the future, it can be replaced by the monoid default value
And creat class for function orEmpty
`implicit class OrEmpty[A](value :Option[A])(implicit defaultValue : DefaultValue[A]) {

def orEmpty : A = value.getOrElse(defaultValue.get)
}`
So the users can easely extend for other type by implementing DefaultValue

This is a brilliant approach. I'll refactor accordingly :)

loicdescotte · 2017-10-27T07:23:33Z

Nice, thanks :)
Can you git rebase (squash) your branch before the merge, so we have a single commit for this ?

sohum2002 · 2017-10-28T17:34:13Z

@loicdescotte Squashed and ready to merge :)

loicdescotte · 2017-10-29T07:25:11Z

Thanks!

sohum2002 mentioned this pull request Oct 18, 2017

Implement orEmpty for Option[T] #35

Closed

dgouyette reviewed Oct 18, 2017

View reviewed changes

loicdescotte reviewed Oct 21, 2017

View reviewed changes

loicdescotte reviewed Oct 22, 2017

View reviewed changes

sohum2002 force-pushed the issue-35 branch from b1374d5 to bc3097e Compare October 23, 2017 17:05

Added orEmpty and orZero functions for Option

78e71a4

sohum2002 force-pushed the issue-35 branch from 6c19082 to 78e71a4 Compare October 27, 2017 07:51

loicdescotte merged commit 24021de into scala-hamsters:master Oct 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue-35] Added orEmpty and orZero functions for Option #44

[Issue-35] Added orEmpty and orZero functions for Option #44

sohum2002 commented Oct 18, 2017

dgouyette commented Oct 18, 2017 •

edited

Loading

dgouyette left a comment

dgouyette Oct 18, 2017

dgouyette Oct 18, 2017

dgouyette Oct 18, 2017

sohum2002 Oct 19, 2017

loicdescotte Oct 21, 2017

sohum2002 commented Oct 18, 2017 •

edited

Loading

loicdescotte Oct 21, 2017 •

edited

Loading

sohum2002 commented Oct 21, 2017

dgouyette commented Oct 21, 2017 •

edited

Loading

loicdescotte Oct 22, 2017

loicdescotte Oct 22, 2017

sohum2002 commented Oct 22, 2017

loicdescotte Oct 22, 2017 •

edited

Loading

sohum2002 Oct 22, 2017

oraclewalid Oct 22, 2017

dgouyette Oct 23, 2017

sohum2002 Oct 23, 2017

loicdescotte commented Oct 27, 2017 •

edited

Loading

sohum2002 commented Oct 28, 2017

loicdescotte commented Oct 29, 2017

		@@ -0,0 +1,22 @@
		package io.github.hamsters

		object OptionHelper {

[Issue-35] Added orEmpty and orZero functions for Option #44

[Issue-35] Added orEmpty and orZero functions for Option #44

Conversation

sohum2002 commented Oct 18, 2017

dgouyette commented Oct 18, 2017 • edited Loading

dgouyette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohum2002 commented Oct 18, 2017 • edited Loading

loicdescotte Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

sohum2002 commented Oct 21, 2017

dgouyette commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohum2002 commented Oct 22, 2017

loicdescotte Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicdescotte commented Oct 27, 2017 • edited Loading

sohum2002 commented Oct 28, 2017

loicdescotte commented Oct 29, 2017

dgouyette commented Oct 18, 2017 •

edited

Loading

sohum2002 commented Oct 18, 2017 •

edited

Loading

loicdescotte Oct 21, 2017 •

edited

Loading

dgouyette commented Oct 21, 2017 •

edited

Loading

loicdescotte Oct 22, 2017 •

edited

Loading

loicdescotte commented Oct 27, 2017 •

edited

Loading