Groovy/Grails code cleanup suggestions, please!

Question

I'm new to Groovy & Grails, and I have a feeling that things don't have to be this ugly... so how can I make this code nicer?

This is a Grails controller class, minus some uninteresting bits. Try not to get too hung up that my Car only has one Wheel - I can deal with that later :-)

changeWheel is an Ajax action.

class MyController {
    ...
    def changeWheel = {
        if(params['wheelId']) {
            def newWheel = Wheel.findById(params['wheelId'])
            if(newWheel) {
                def car = Car.findById(params['carId'])
                car?.setWheel(newWheel)
                if(car?.save()) render 'OK'
            }
        }
    }
}

Answer 1

I'd actually start using Command Objects.

Try this:

class MyController {
    def index = {
    }
    def changeWheel = { CarWheelCommand cmd ->
        if(cmd.wheel && cmd.car) {
            Car car = cmd.car
            car.wheel = cmd.wheel
            render car.save() ? 'OK' : 'ERROR'
        } else {
            render "Please enter a valid Car and wheel id to change"
        }
    }
}
class CarWheelCommand {
    Car car
    Wheel wheel
}

and then in your view use 'car.id' and 'wheel.id' instead of 'carId' and 'wheelId'

Answer 2

1) pull

params['wheelId'] 

and

params['carId']

out into their own defs

2) multiple nested ifs is never optimal. You can get rid of the outermost one by having a validateParams method and rendering some sort of response if wheelId and carId are not set. Or just do

if (carId == null || wheelId == null) {
    // params invalid
}

3) Assuming everything is ok you could just do

def newWheel = Wheel.findById...
def car = Car.findById...
if (car != null && newWheel != null) {
    car.setWheel(newWheel)
    car.save()
    render 'OK'
} else {
   // either wheel or car is null

}

this gets rid of more nested structures...

4) finally, to make the code self documenting, you can do things like assign the conditional tests to appropriately named variables. So something like

def carAndWheelOk = car != null && newWheel != null
if (carAndWheelOk) {
   // do the save
} else {
   // car or wheel not ok
}

this might be overkill for two tests, but you only are taking care of one wheel here. If you were dealing with all 4 wheels, this type of things increases readability and maintainability.

Note that this advice works in any language. I don't think you can do too much with groovy's syntactic sugar, but maybe some groovy gurus can offer better advice.