Groovy method signature strategies

Published on

After a bug I recently introduced to production was fixed, I set out to write about what happened as a sort of post-mortem exercise. That ends up being covered mostly in the last section of this post, but found some other best practice stuff buried in a readme file I had written a while back that was also relevant, so just posting that all together, ‘cause why not.

In Grails services specifically I have found that it is typically better to be a bit more Java like when possible. Groovy lets you just def everything, or even omit types declarations altogether and that can be great at times. I don’t tend to spend much time fighting with the compiler in Groovy. In Grails the service layer is where a lot of your business logic is supposed to live. Methods here tend to be reused or are set up to be used by multiple controllers or whatever. Being a bit more explicit with defining the method signature can be very helpful, when used with @CompileStatic this constitutes a sort of test that automatically runs at compile time as well.

Here is what that looks like.

Include types

Whenever possible specify the return type of service methods and the types of arguments they expect.

❌ Bad

	def isThisThingLegit(thing, user)

✅ Good

	Boolean isThisThingLegit(Thing thing, AppUser user)

Naming

Good naming is just generally good practice - this is not Groovy or Grails specific of course - but given that Java programs are often overly or painfully verbose in their naming, I have seen developers in Grails sometimes over-correct, naming things to way to vaguely. Mostly refering to my past self here…

The naming stuff could probably go without saying except that Grails sits in this spot where a lot of the people I have worked alongside in Grails projects came either from either a Java background, a Rails background, or a JavaScript background, and those camps seem to clash a bit awkardly when it comes to working in Grails together.

❌ Bad

	Boolean allGood(Thing arg1, Map params)

✅ Better at least

	Boolean isThisThingLegit(Thing thing, Map propertiesToCheck)

Map params as an argument to a service method is something I have frequently seen, and drives me a bit crazy now. I know the root of this is in part because Grails controllers use that for request parameters, and just passing that whole thing along to a service method can be tempting, but the type of Map and name of params gives the person working on the service method no useful information about what should be in that map. It’s even worse if no type is specified at all, which Grails totally allows!

In this method params could literally be anything 😢

	def sendAnEmail(body, params) {...}

To be fair so could body, but at least the service class name or the method name might give a hint as to what that would be.

Adding a type can inform or contextualize the name, but don’t stop at that, naming arguments well can help a lot!

	def sendAnEmail(String body, String[] recipients) {...}

Good naming is at least as much art as science.

Avoid problems with inferred returns

The last statement of a method is inferred as a return value in Groovy.

This can bite you. Hard. But some static typing along with @CompileStatic or @TypeChecked or @GrailsCompileStatic - discussed a bit more here - can save you some serious headaches

Be explicit with your return statements when its not obvious.

There’s no need to use return if the method is only one or two lines. But be aware of how the implicit return works

// Here is a casting error waiting to happen
Document doSomethingToTheDocument(Document doc, Map stuffToAppendMaybe) {
	...
	doc.prop = 'foo'
	if (weDidAnything) validateTheThingsYouDidWereOkay(doc) // if this returns a boolean... things go boom
	else doc.flag = bar
}

// You can avoid the casting error by having a generic return type
// - def is just an Object - but the method having no defined return type
// in the method signature leaves it vulnerable to other issues
def doSomethingToTheDocument(Document doc, Map stuffToAppendMaybe) {
	...
}

Use void methods when you don’t need anything returned.

Becuase Groovy’s return is implicit you have to be explicit about wanting no return. This is a stark difference with JavaScript, for example. There, if you forget to add a return the default is you get nothing. Coffeescript was more like Groovy in this regard and a few others, and though I liked Coffeescript, it’s implicit return was a contentious thing, and I have become a bit less fond of it in Groovy over time too. But a thing Groovy has that overcomes the biggest problem of the implicit return is the void

specifying void in the signature avoids unintentional casting errors in addition to any unnecessary casting overhead.

void doSomethingToTheDocument(Document doc, Map stuffToAppendMaybe) {
	...
}

Multiple method signatures

Taking advantage of static typing in Grails is often surprisingly painless and has many benefits, but it isn’t completely without tradeoffs. One benefit of less strict method signatures is that you don’t need repeated methods with subtley different signatures to deal with different argument types.

Date parseToDate(date) {...}

Inside that one method you can handle many cases - wether date was already a Date, if it was a String, or if it was a Long - as well as if it were null.

If you sepecify a default value you can even avoid defensive null checking!

Date parseToDate(date='') {...}

However, if you want to move to

Date parseToDate(String date) {...}

you would also need

Date parseToDate(Date date) {...}

and

Date parseToDate(Long date) {...}

to handle those same range of cases.

Here is the rub, if you left it at just those though you could have a problem. In fact, this is where my code did have a problem. If your incoming date is null Groovy will not know what to do will throw an error

groovy.lang.GroovyRuntimeException: Ambiguous method overloading for method ...
Cannot resolve which method to invoke for [null] due to overlapping prototypes between:
	[class java.lang.String]
	[class java.util.Date]

To prevent this you can add one more method.

Date parseToDate(Object date) {return null}

The ambibuty is replaced with a specificity cascade. The null situation falls into the less specific signature, and the others go to the one more fitting for their specified types.

So is the static typing here worth it?

And here we have one of those classic “it depends” scenarios.

Factors: