2

(I am currently using groovy but it should apply to most OO languages so I also put the langauge-agnostic tag)

I try to program in a function style which also includes method chaining and avoiding variables. Therefore I often find myself writing code with the with method in groovy that every object has. It works likes this:

someobject
.doSomething()
.doEvenMore() //this results in an object that I need check for some condition, e.g. a String
.with { String result ->
    if (result != "I AM A CORRECT RESULT")
        throw new Exception("assertion failed")
    else
        return result  //need to do this because else the closure will return null
}
.doAnotherThingOnTheResult()
//and so on

(See also https://codereview.stackexchange.com/questions/57676/better-way-to-assert-correct-return-values-in-groovy for a real life example I asked some time ago)

This is rather unconcise so I was searching for a better way to check for conditions without using to have with. I came up with an idea I would like to hear your opinions to. That is, an method that all objects have and that can be used like this:

someobject
.doSomething()
.doEvenMore() //this results in an object that I need check for some condition, e.g. a String
.assertTrue (new Exception("custom exception")) { it == "I AM A CORRECT RESULT" }
.doAnotherThingOnTheResult()
//and so on

or for default behaviour with a default exception type

someobject
.doSomething()
.doEvenMore() //this results in an object that I need check for some condition, e.g. a String
.assertTrue { it == "I AM A CORRECT RESULT" } //results in AssertionException or sth similiar
.doAnotherThingOnTheResult()
//and so on

In groovy I can make it that every (new) created object is decorated with such a method. What do you guys think about it? Are there better ways or should I stay with the with method?

  • I don't know Groovy, but won't your assertTrue method create an Exception every time it's invoked, even if it's not needed? That aside, why can't you hide the boilerplate of if/else behind a wrapper function? – Doval Nov 24 '14 at 15:45
  • @Doval assertTrue would check the return value of the given function ({...} is an anonymous function here) and only throw the default/given exception if the value is false. What exactly do you mean by a wrapper function? If I get you right, I would have to write a different wrapper function everytime. I'd like to avoid that. – valenterry Nov 24 '14 at 15:53
  • I get that assertTrue takes an anonymous function, but I'm referring to the new Exception("custom exception") argument. Wouldn't that get evaluated each time? And why would you need to write a new wrapper each time? I assume the if/else boilerplate is the same each time. – Doval Nov 24 '14 at 16:07
  • @Doval Ah, no. The Exception is one of the arguments of assertTrue. It must be a throwable and is then thrown if the given closure (2nd argument) returns false on the object of the assertTrue. If the if/else is the same, right, then I need only one wrapper. But it will be almost different each time. Here I might check, that the called function returns a String with some content. Then somewhere else I want to assert, that the size of the collection is 5. And so on. – valenterry Nov 24 '14 at 17:40
  • 1
    What I'm getting at is that you appear to be creating an exception with every call to assertTrue, even if it's not thrown. That's a relatively expensive operation unless your custom exception explicitly disables the stack trace. As for the wrapper, it can take a function that does the boolean check as an argument. E.g. .with(assertTrue(...lambda that checks boolean condition here...)) and assertTrue contains the if/else logic. Is that not possible? – Doval Nov 24 '14 at 17:45
1

First, I think it is a good idea to refactor the with part into its own function. It makes IMHO the code easier to read, and it corresponds to the "Single Level of Abstraction" principle, which is one characteristic of clean code.

Second, if it is a good idea to decorate every class of your program with this additional method, depends. I would do this only if you need this function all over your whole program, in many classes. Otherwise, I would restrict it to the classes which really use the method. This helps to avoid unintentional naming collisions.

3

I'm on the fence whether your code should be called a fluent interface or a train wreck. Either way, method chaining should not be overused because it decreases readability. I have first hand experience from a project where I found myself writing wrapper functions to shorter some chains. Furthermore, some folks such as Robert C. Martin in "Clean Code" Chapter 3 suggest that one should strive to separate commands from queries, which your code clearly does not do. I don't say this is an absolute truth, but you should be more careful with method chaining. After all, functional style does not say that one must chain everything but rather that one should strive to build code from functions that do not use implicit state. So, of course you should follow Doc Brown's suggestion and refactor with into a separate function, but you should also check if you're overusing method chaining.

For example, it may be better to assign the result of computation before invariant check to a variable, then verify the variable satisfies the check, and then continue with computation. You may even decide to wrap several commonly used sub-chains into separate functions with a descriptive name.

If it was Haskell, you could have returned Maybe.Some(yourobj) from each method and rely on compiler to halt computation as soon as Maybe.None is returned. This way, a verification method would return yourobj unchanged if it is OK, and return None otherwise. In Java there is a very similar thing called Optional, and this article explains how it can be used to solve your issue in a different way.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.