Groovy / Grails code cleaning guidelines, please!

Question

I'm new to Groovy and Grails, and I have a feeling that everything should not be so ugly ... so how can I make this code better?

This is the Grails controller class, minus some uninteresting bits. Try not to depend too much on the fact that my Car has only one Wheel - I can figure it out 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 would start using Command Objects .

Try the following:

 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'] 

into your own defs

2) multiple nested ifs are never optimal. You can get rid of the outermost by having the validateParams method and rejecting some kind of answer if wheelId and carId are not set. Or simply

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

3) Assuming everything is fine, 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 eliminates more nested structures ...

4) finally, in order to make the code self-declared, you can do such things as assigning conditional tests to the corresponding names. So something like

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

this may be redundant for two tests, but you only care about one wheel here. If you have dealt with all 4 wheels, this type of thing improves readability and maintainability.

Please note that this tip works in any language. I don’t think you can do too much with groovy syntactic sugar, but perhaps some groovy gurus can offer better advice.

Answer 3

There are a few things you could do, such as moving some code to a service or command object. But without changing the structure too much, I (subjectively) think that the following will facilitate reading the code:

• use notation notation instead of indexing the array for the values ​​of the reference parameters (params.wheelId instead of params ['wheelId'])

• I would invert if to reduce nesting, I think it makes it clearer what these exceptions are.

For instance:

 if(!params.wheelId) { sendError(400, "wheelId is required") return } .... .... if(!newWheel) { sendError(404, "wheel ${params.wheelId} was not found.") return } 

Now, if you don't mind changing the structure and adding more lines of code ...

The act of changing the wheel may not be common for just one controller action. In this case, I would recommend putting the GORM / database logic in a service class. Then your controller should only verify that it has the correct params inputs and passes them to the Service for the actual bus replacement. The "Service" method may be the transactional one you need in case you have to dismantle the old bus before installing a new one.

In the Service, I would choose exceptions for exceptional cases, for example, when the wheel is not found, the car is not found, or if an error occurs in changing the tire. Then your controller can catch them and respond with the appropriate HTTP status codes.