Groovy/Grails Code Cleanup Vorschläge, bitte!

Question

Ich bin neu in Groovy & Grails, und ich habe das Gefühl, dass die Dinge nicht so hässlich sein müssen ... also wie kann ich diesen Code schöner machen?

Dies ist eine Grails-Controller-Klasse, abzüglich einiger uninteressanter Bits. Versuchen Sie, nicht zu hängen, dass meine Car nur eine Wheel hat - ich kann damit später umgehen :-)

changeWheel ist eine Ajax Aktion.

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

1) ziehen

params['wheelId'] 

und

params['carId'] 

in eigene defs

2) mehrere verschachtelte ifs ist nie optimal. Sie können die äußerste entfernen, indem Sie eine validateParams-Methode verwenden und eine Art von Antwort ausgeben, wenn wheelId und carId nicht gesetzt sind. Oder haben nur

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

3) Unter der Annahme, alles in Ordnung ist kann man nur tun

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 

} 

dies von mehr verschachtelte Strukturen entledigt ...

4) schließlich, um den Code selbst zu machen Dokumentieren können Sie Dinge wie die Zuordnung der bedingten Tests zu entsprechend benannten Variablen. So etwas wie

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

Dies könnte Overkill für zwei Tests sein, aber Sie sind nur für ein Rad hier. Wenn Sie mit allen 4 Rädern beschäftigt sind, erhöht diese Art von Dingen Lesbarkeit und Wartbarkeit.

Beachten Sie, dass dieser Hinweis in jeder Sprache funktioniert. Ich denke nicht, dass du mit dem syntaktischen Zucker von Groovy zu viel anfangen kannst, aber vielleicht können einige groovige Gurus bessere Ratschläge geben.

Answer 2

Ich würde tatsächlich anfangen zu verwenden Command Objects.

Versuchen Sie folgendes:

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 
} 

und dann Ihrer Ansicht nach Gebrauch ‚car.id‘ und ‚wheel.id‘ statt ‚carId‘ und ‚wheelId‘

Answer 3

Es gibt ein paar Dinge, die Sie tun können, wie Verschieben Sie einen Code in ein Service- oder Befehlsobjekt. Aber ohne die Struktur zu verändern, zu viel, ich (subjektiv) denken, die folgenden würde der Code machen leichter zu lesen:

1. Verwendung Punktnotation anstelle von Array-Indizierung params Werte (params.wheelId statt params zu verweisen [‘ WheelId '])

2. Ich würde invertieren, wenn die Verschachtelung zu reduzieren, ich denke, dies macht es klarer, was die Ausnahmen sind.

Zum Beispiel:

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

Nun, wenn Sie die Struktur zu ändern und mehr Zeilen Code nichts ausmacht Hinzufügen ...

Der Akt der Radwechsel kann eine gemeinsame sein Auftreten über mehr als nur eine Controller-Aktion. In diesem Fall würde ich empfehlen, die GORM/Datenbank-Logik in eine Service-Klasse zu stellen. Dann muss Ihr Controller nur verifizieren, dass er die korrekten Parameter Eingänge hat und diese an den Service weitergeben, um den tatsächlichen Reifenwechsel durchzuführen. Eine Service-Methode kann transaktional sein, was Sie in dem Fall wünschen, in dem Sie möglicherweise den alten Reifen abmontieren müssen, bevor Sie den neuen montieren.

In den Service würde ich Ausnahmen für Ausnahmefälle werfen, wenn ein Rad nicht gefunden wird, ein Auto nicht gefunden wird, oder wenn ein Fehler beim Reifenwechsel auftritt. Dann kann Ihr Controller diese abfangen und mit den richtigen HTTP-Statuscodes antworten.