Permalink
Browse files

Enhanced the opionation of Box

  • Loading branch information...
1 parent f4c20ac commit 0e1e2a390f18c467267a04eefdcdb0e160d28786 @dpp dpp committed Jul 31, 2012
Showing with 112 additions and 34 deletions.
  1. +112 −34 core/common/src/main/scala/net/liftweb/common/Box.scala
@@ -206,6 +206,33 @@ sealed abstract class Box[+A] extends Product with Serializable{
*/
def isDefined: Boolean = !isEmpty
+ /**
+ * Yeah, Yeah, Yeah... you think it's alright to access references that may or may not be legal to access...
+ * well, it's really stupid idea.
+ *
+ * One of the major points of using Scala is to avoid things like Null Pointer Exceptions and Box
+ * is a key way to do this. You can access the value of the Box (if and only if the Box contains a value)
+ * with a for comprehension or with the map, flatMap and foreach methods. Using this method,
+ * however is the worst way to access to contents of the Box.
+ *
+ * But, but, but you say, this causes a fail-fast exception.
+ *
+ * Dude... you don't want exceptions in your code. Exceptions are for exceptional situations such as
+ * your disk failing or something on an external system. Exceptions are not about the logic of your application.
+ * Your logic should work with all input values in your application.
+ *
+ * This method name reflects the design quality related to using this method. It's also easy to
+ * find by grep'ing through your code... and when people see the method call in a code review... well...
+ *
+ * @param nameOfPersonThatThoughtThisWasAGoodIdea -- yes... your name will be included in every exception
+ * thrown by calling this method
+ * @param lameButSemiPlausableReasonWhyYouThinkThisIsAGoodIdea -- Okay, Mr. Smartypants, you think it's a good idea
+ * calling this method. Tell us why.
+ * @return The contents of the Box if it has one or an exception if not
+ */
+ def openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod(nameOfPersonThatThoughtThisWasAGoodIdea: String,
+ lameButSemiPlausableReasonWhyYouThinkThisIsAGoodIdea: String): A
+
/**
* Return the value contained in this Box if it is Full;
* throw an exception otherwise.
@@ -221,8 +248,8 @@ sealed abstract class Box[+A] extends Product with Serializable{
*
* @return the value contained in this Box if it is full; throw an exception otherwise
*/
- @deprecated("Don't use this method unless you are guaranteed of a Full Box", "2.4")
- def open_! : A
+ @deprecated("use openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod, or better yet, do the right thing with your code and use map, flatMap or foreach", "2.4")
+ final def open_! : A = openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod("dpp", "Implementing the deprecated method that you shouldn't be using")
/**
* Return the value contained in this Box if it is Full;
@@ -235,7 +262,8 @@ sealed abstract class Box[+A] extends Product with Serializable{
*
* @return the value contained in this Box if it is full; throw an exception otherwise
*/
- def openTheBox: A = this.open_!
+ @deprecated("use openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod, or better yet, do the right thing with your code and use map, flatMap or foreach", "2.4")
+ final def openTheBox: A = openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod("dpp", "Implementing the deprecated method that you shouldn't be using")
/**
* Return the value contained in this Box if it is full; otherwise return the specified default
@@ -480,19 +508,34 @@ final case class Full[+A](value: A) extends Box[A]{
def isEmpty: Boolean = false
-
+
/**
- * Return the value contained in this Box if it is Full;
- * throw an exception otherwise.
- * The method has a '!' in its name. This means "don't use it unless
- * you are 100% sure that the Box is Full and you should probably
- * comment your code with the explanation of the guaranty.
- * The better case for extracting the value out of a Box can
- * be found at http://lift.la/scala-option-lift-box-and-how-to-make-your-co
+ * Yeah, Yeah, Yeah... you think it's alright to access references that may or may not be legal to access...
+ * well, it's really stupid idea.
*
- * @return the value contained in this Box if it is full; throw an exception otherwise
+ * One of the major points of using Scala is to avoid things like Null Pointer Exceptions and Box
+ * is a key way to do this. You can access the value of the Box (if and only if the Box contains a value)
+ * with a for comprehension or with the map, flatMap and foreach methods. Using this method,
+ * however is the worst way to access to contents of the Box.
+ *
+ * But, but, but you say, this causes a fail-fast exception.
+ *
+ * Dude... you don't want exceptions in your code. Exceptions are for exceptional situations such as
+ * your disk failing or something on an external system. Exceptions are not about the logic of your application.
+ * Your logic should work with all input values in your application.
+ *
+ * This method name reflects the design quality related to using this method. It's also easy to
+ * find by grep'ing through your code... and when people see the method call in a code review... well...
+ *
+ * @param nameOfPersonThatThoughtThisWasAGoodIdea -- yes... your name will be included in every exception
+ * thrown by calling this method
+ * @param lameButPlausableReasonWhyYouThinkThisIsAGoodIdea -- Okay, Mr. Smartypants, you think it's a good idea
+ * calling this method. Tell us why.
+ * @return The contents of the Box if it has one or an exception if not
*/
- def open_! : A = value
+ def openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod(nameOfPersonThatThoughtThisWasAGoodIdea: String,
+ lameButPlausableReasonWhyYouThinkThisIsAGoodIdea: String): A = value
+
override def openOr[B >: A](default: => B): B = value
@@ -564,17 +607,34 @@ sealed abstract class EmptyBox extends Box[Nothing] with Serializable {
def isEmpty: Boolean = true
/**
- * Return the value contained in this Box if it is Full;
- * throw an exception otherwise.
- * The method has a '!' in its name. This means "don't use it unless
- * you are 100% sure that the Box is Full and you should probably
- * comment your code with the explanation of the guaranty.
- * The better case for extracting the value out of a Box can
- * be found at http://lift.la/scala-option-lift-box-and-how-to-make-your-co
+ * Yeah, Yeah, Yeah... you think it's alright to access references that may or may not be legal to access...
+ * well, it's really stupid idea.
*
- * @return the value contained in this Box if it is full; throw an exception otherwise
+ * One of the major points of using Scala is to avoid things like Null Pointer Exceptions and Box
+ * is a key way to do this. You can access the value of the Box (if and only if the Box contains a value)
+ * with a for comprehension or with the map, flatMap and foreach methods. Using this method,
+ * however is the worst way to access to contents of the Box.
+ *
+ * But, but, but you say, this causes a fail-fast exception.
+ *
+ * Dude... you don't want exceptions in your code. Exceptions are for exceptional situations such as
+ * your disk failing or something on an external system. Exceptions are not about the logic of your application.
+ * Your logic should work with all input values in your application.
+ *
+ * This method name reflects the design quality related to using this method. It's also easy to
+ * find by grep'ing through your code... and when people see the method call in a code review... well...
+ *
+ * @param nameOfPersonThatThoughtThisWasAGoodIdea -- yes... your name will be included in every exception
+ * thrown by calling this method
+ * @param lameButPlausableReasonWhyYouThinkThisIsAGoodIdea -- Okay, Mr. Smartypants, you think it's a good idea
+ * calling this method. Tell us why.
+ * @return The contents of the Box if it has one or an exception if not
*/
- def open_! = throw new NullPointerException("Trying to open an empty Box")
+ def openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod(nameOfPersonThatThoughtThisWasAGoodIdea: String,
+ lameButPlausableReasonWhyYouThinkThisIsAGoodIdea: String) =
+ throw new NullPointerException("So, "+nameOfPersonThatThoughtThisWasAGoodIdea+" thought it might be a good idea to open a Box even though it could be empty. The justification was '"+lameButPlausableReasonWhyYouThinkThisIsAGoodIdea+"'. Turns out it was a bad idea")
+
+
override def openOr[B >: Nothing](default: => B): B = default
@@ -604,20 +664,38 @@ object Failure {
sealed case class Failure(msg: String, exception: Box[Throwable], chain: Box[Failure]) extends EmptyBox{
type A = Nothing
+
/**
- * Return the value contained in this Box if it is Full;
- * throw an exception otherwise.
- * The method has a '!' in its name. This means "don't use it unless
- * you are 100% sure that the Box is Full and you should probably
- * comment your code with the explanation of the guaranty.
- * The better case for extracting the value out of a Box can
- * be found at http://lift.la/scala-option-lift-box-and-how-to-make-your-co
+ * Yeah, Yeah, Yeah... you think it's alright to access references that may or may not be legal to access...
+ * well, it's really stupid idea.
*
- * @return the value contained in this Box if it is full; throw an exception otherwise
- */
- override def open_! = throw new NullPointerException("Trying to open a Failure Box: " + msg) {
- override def getCause() = exception openOr null
- }
+ * One of the major points of using Scala is to avoid things like Null Pointer Exceptions and Box
+ * is a key way to do this. You can access the value of the Box (if and only if the Box contains a value)
+ * with a for comprehension or with the map, flatMap and foreach methods. Using this method,
+ * however is the worst way to access to contents of the Box.
+ *
+ * But, but, but you say, this causes a fail-fast exception.
+ *
+ * Dude... you don't want exceptions in your code. Exceptions are for exceptional situations such as
+ * your disk failing or something on an external system. Exceptions are not about the logic of your application.
+ * Your logic should work with all input values in your application.
+ *
+ * This method name reflects the design quality related to using this method. It's also easy to
+ * find by grep'ing through your code... and when people see the method call in a code review... well...
+ *
+ * @param nameOfPersonThatThoughtThisWasAGoodIdea -- yes... your name will be included in every exception
+ * thrown by calling this method
+ * @param lameButPlausableReasonWhyYouThinkThisIsAGoodIdea -- Okay, Mr. Smartypants, you think it's a good idea
+ * calling this method. Tell us why.
+ * @return The contents of the Box if it has one or an exception if not
+ */
+ override def openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod(nameOfPersonThatThoughtThisWasAGoodIdea: String,
+ lameButPlausableReasonWhyYouThinkThisIsAGoodIdea: String) =
+ throw new NullPointerException("So, "+nameOfPersonThatThoughtThisWasAGoodIdea+
+ " thought it might be a good idea to open a Box even though it could be empty. The justification was '"+
+ lameButPlausableReasonWhyYouThinkThisIsAGoodIdea+"'. Turns out it was a bad idea. By the way, the Failure that was opened contained the error message: '"+msg+"'.") {
+ override def getCause() = exception openOr null
+ }
override def map[B](f: A => B): Box[B] = this

19 comments on commit 0e1e2a3

👍

Member

lkuczera replied Jul 31, 2012

Haha this is funny but open is useful sometimes. Like I have places in code that are guaranteed to have user logged in (otherwise its better to throw exception than to proceed). So having such long method name in my code doesn't make me happy.

Member

pbrant replied Jul 31, 2012

-1

open_! is actually useful in some places, especially tests

A good framework should give programmers good tools and trust them to use them well, not try to insult them with function names. This CL should be reverted.

Owner

dpp replied Jul 31, 2012

The only place I use open_! is where the code path is guaranteed to work and I generally document that code path.

I like Peter's openOrDie pimp and it might be a good module.

But in general, building code around exceptions as part of the logic of the code is not a good practice and really should be avoided.

I certainly agree with your "in general" principal, but it's up to the end programmer to know when she's not in the general case. When she's not a concise method name like open_! is much better than a very verbose one like openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod that will only cause confusion. The _! is a fine signifier that care should be taken.

Owner

dpp replied Jul 31, 2012

One more thing... you can always use Box.get:

scala> import net.liftweb.common._
import net.liftweb.common._

scala> var x: Box[Int] = Full(33)
x: net.liftweb.common.Box[Int] = Full(33)

scala> x.get
res0: Int = 33

scala> x = Empty
x: net.liftweb.common.Box[Int] = Empty

scala> x.get
java.util.NoSuchElementException: None.get
    at scala.None$.get(Option.scala:274)
    at scala.None$.get(Option.scala:272)
    at .<init>(<console>:12)
    at .<clinit>(<console>)
    at .<init>(<console>:11)
    at .<clinit>(<console>)
    at $print(<console>)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at scala.tools.nsc.interpreter.IMain$ReadEvalPrint.call(IMain.scala:704)
    at scala.tools.nsc.interpreter.IMain$Request.loadAndRun(IMain.scala:914)
    at scala.tools.nsc.interpreter.IMain.loadAndRunReq$1(IMain.scala:546)
    at scala.tools.nsc.interpreter.IMain.interpret(IMain.scala:577)
    at scala.tools.nsc.interpreter.IMain.interpret(IMain.scala:543)
    at scala.tools.nsc.interpreter.ILoop.reallyInterpret$1(ILoop.scala:694)
    at scala.tools.nsc.interpreter.ILoop.interpretStartingWith(ILoop.scala:745)
    at scala.tools.nsc.interpreter.ILoop.command(ILoop.scala:651)
    at scala.tools.nsc.interpreter.ILoop.processLine$1(ILoop.scala:542)
    at scala.tools.nsc.interpreter.ILoop.loop(ILoop.scala:550)
    at scala.tools.nsc.interpreter.ILoop.process(ILoop.scala:822)
    at scala.tools.nsc.interpreter.ILoop.main(ILoop.scala:851)
    at xsbt.ConsoleInterface.run(ConsoleInterface.scala:57)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at sbt.compiler.AnalyzingCompiler.call(AnalyzingCompiler.scala:57)
    at sbt.compiler.AnalyzingCompiler.console(AnalyzingCompiler.scala:48)
    at sbt.Console.console0$1(Console.scala:23)
    at sbt.Console$$anonfun$apply$2$$anonfun$apply$1.apply$mcV$sp(Console.scala:24)
    at sbt.TrapExit$.executeMain$1(TrapExit.scala:33)
    at sbt.TrapExit$$anon$1.run(TrapExit.scala:42)


scala> 

Because Box has an implicit conversion to Option.

Also, Lift has a long history of method names that discourage use:

MetaMapper.scala:

def findAllByInsecureSql(query: String, checkedBy: IHaveValidatedThisSQL): List[A]
Contributor

Dridus replied Jul 31, 2012

One more thing... you can always use Box.get:

True, but that's actually worse not better so definitely shouldn't be encouraged.

Also, Lift has a long history of method names that discourage use

"findAllByInsecureSql" is a fine method name. It's still concise and is actually a very important warning because if care is not taken here you could end up with a very significant security hole.

openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod is 3x as many characters, is in no way concise, and comes with editorial you don't see in findAllByInsecureSql. This method is also far less dangerous than findAllByInsecureSql.

👍

Owner

dpp replied Jul 31, 2012

"findAllByInsecureSql" is a fine method name. It's still concise and is actually a very important warning because if care is not taken here you could end up with a very significant security hole.

openTheBoxButThisMightResultInAnExceptionSoDontUseThisMethod is 3x as many characters, is in no way concise, and comes with editorial you don't see in findAllByInsecureSql. This method is also far less dangerous than findAllByInsecureSql.

Okay... I'm cool with changing the method name to something shorter, but I'd like to keep the justification parameter because the way I encouraged the use of open_! is with a justification message which nobody ever includes.

How about openOrThrowException(justification: String)?

Owner

dpp replied Jul 31, 2012

Oh...

One more thing... you can always use Box.get:

True, but that's actually worse not better so definitely shouldn't be encouraged.

Agreed it's worse, but it's a secret handshake... once you know about implicit conversions, etc., you're more likely to get the use of open_! correct.

openOrThrowException(justification: String) seems reasonable

or perhaps just

openOrThrow(justification: String)

Owner

Shadowfiend replied Jul 31, 2012

+1 for openOrThrow. I get the reasoning, but I agree it's a bit comically long as it stands. I think openOrThrow conveys everything that needs to be conveyed concisely.

Owner

dpp replied Jul 31, 2012

Here's the updated commit:

077e0db

I chose openOrThrowException because I wanted the word "Exception" to be part of the method description and typing a few extra
letters (for those who are not using code completion) in this case discourages use of the method. I don't think concise is a virtue for
this method, although I agree that obnoxious is not a virtue for the method either.

Contributor

nafg replied Aug 1, 2012

Contributor

nafg replied Aug 1, 2012

Member

lkuczera replied Aug 1, 2012

+1 for openOrThrowException(justification: String)

IMO a dangerous-sounding name, such as unsafeOpen, is a better warning than a ridiculously long name. After all, ridiculouslyLongNamesNeverDiscouragedEnterpriseJavaDevelopers!

Please sign in to comment.