Ways to improve this code

Question

I am trying to write some test code for my java application using Scalatest. I figured, since Scala has so much more readable syntax it would result with more readable test code.

So far, this is what I managed:

package com.xyz

import org.scalatest.FlatSpec
import org.scalatest.matchers.ShouldMatchers
import com.xyz.SecurityService
import org.mockito.Mockito._
import org.scalatest.mock.MockitoSugar
import org.mockito.Matchers._
import javax.servlet.jsp.tagext.Tag

class CheckRoleTagSpec extends FlatSpec with ShouldMatchers with  MockitoSugar {

  behavior of "CheckRole tag"

  it should "allow access when neither role nor root defined" in {
    val securityServiceMock = mock[SecurityService]

    val tag = new CheckRoleTag()
    tag.setSecurityService(securityServiceMock)

    tag.setGroup("group")
    tag.setPortal("portal")

    tag.setRoot(false)
    tag.setRole(null)

    tag.doStartTag should be(Tag.SKIP_BODY)
  }

}

I am quite dissapointed with this code. It's practically the same thing I would need to write in Java. Please help me make it more scala-like and functional.

Answer 1

The code below creates new anonymous class, but doStartTag returns result as expected:

...
(new CheckRoleTag{
   setSecurityService(mock[SecurityService])
   setGroup("group")
   setPortal("portal")
   setRoot(false)
   setRole(null)
} doStartTag) should be(Tag.SKIP_BODY)
...

Answer 2

Since this particular test is just calling a bunch of setters on a object implemented in java, there's not a whole lot you can do to make it more succinct or functional or scalaish. You could remove some repetition with something like

it should "allow access when neither role nor root defined" in {
  val securityServiceMock = mock[SecurityService]

  val tag = new CheckRoleTag()

  locally { 
    import tag._
    setSecurityService(securityServiceMock)
    setGroup("group")
    setPortal("portal")
    setRoot(false)
    setRole(null)
  }

  tag.doStartTag should be(Tag.SKIP_BODY)
}

I'm not sure whether it's really worth it in this case.

Answer 3

You cannot fix an ugly test by rewriting the test. You can only fix it by redesigning the API that is being tested.

Well, technically, it is possible to write ugly tests for good APIs if you try really hard, are very evil, very, stupid, very drunk or very tired. But writing an ugly test takes effort and programmers are lazy, so it is very unlikely that someone would write an ugly test by choice. It is almost impossible to write ugly tests: you stick something in, you get something out, you check whether you got what you expected. That's it. There's really nothing to uglify there.

A test just uses the API the same way a user of the API would use it. It's basically an example of how to use the API properly, that also, almost as a side-effect, happens to check that the API is actually implemented properly. That's why an ugly test is a good indicator for bad API design, and it's also why test-driving the API design is a good thing, even if you don't do TDD.

In this particular case, I can see quite a few ways to improve the API, although these suggestions are necessarily incomplete, shallow and simplistic (not to mention probably wrong), since I don't know anything about your domain:

• Better Names: setRoot sounds like it is setting the root. But, unless false is the root of your hierarchy, I assume that what it is actually setting is whether or not this Tag is the root. So, it should rather be named isRoot or makeRoot or setIsRoot or something like that.
• Better Defaults: continuing with setRoot, assuming that my guess is correct and this sets whether or not the Tag is the root, then the default is the wrong way around. By the very definition of the concept of "root", there can only ever be one root. So, you are forcing your users to specify setRoot(false) every single time, except for that one time when they are actually defining a root. Non-root Tags should be the default, and you should only be forced to setRoot(true) for that one Tag which actually is the root.
• Better Defaults, Part II: setRole(null). Seriously? You are forcing your users to explicitly set the role to be unset? Why not simply make unset the default? After all, the test is called "... when neither role nor root defined", so why define them?
• Fluent API / Builder Pattern: if you really have to construct invalid objects (but see next point), at least use something like a Fluent API or a Builder Pattern.
• Only Construct Valid Objects: But really, objects should always be valid, complete and fully configured, when constructed. You shouldn't have to construct an object, and then configure it.

That way, the test basically becomes:

package com.xyz

import org.scalatest.FlatSpec
import org.scalatest.matchers.ShouldMatchers
import com.xyz.SecurityService
import org.mockito.Mockito._
import org.scalatest.mock.MockitoSugar
import org.mockito.Matchers._
import javax.servlet.jsp.tagext.Tag

class CheckRoleTagSpec extends FlatSpec with ShouldMatchers with MockitoSugar {
  behavior of "CheckRole tag"
  it should "allow access when neither role nor root defined" in {
    val tag = new CheckRoleTag(mock[SecurityService], "group", "portal")

    tag.doStartTag should be(Tag.SKIP_BODY)
  }
}