7
\$\begingroup\$

I'm completely new to Groovy, but have many years of Java experience, and recently I have started working on a Grails application, currently it contains the following:

  • User and Person domain classes
  • UserService service class
  • UserController controller class to offer a REST API

This application is written using Groovy 2.4.3 and Grails 3.0.2.

I'd like to have this code reviewed and if possible extra attention on the transition from Java to Groovy and on the mixture of Grails and Spring code.

domain/User.groovy

class User {
    static constraints = {
        email email: true, unique: true
        person nullable: true
        registrationDate nullable: true
        loginDate nullable: true
    }

    String email
    String password
    Person person
    Instant registrationDate
    Instant loginDate
}

domain/Person.groovy

class Person {
    static constraints = {
        middleName nullable: true
        city nullable: true
        street nullable: true
        houseNumber nullable: true
        zipCode nullable: true
        email email: true, nullable: true
        bankAccount nullable: true
    }

    static hasMany = [
        telephoneNumbers: String
    ]

    String firstName
    String middleName
    String lastName
    String city
    String street
    String houseNumber
    String zipCode
    String email
    List<String> telephoneNumbers
    String bankAccount
}

conf/spring/resources.groovy

beans = {
    clock(Clock) { bean ->
        bean.factoryMethod = "systemDefaultZone"
    }
}

services/UserService.groovy

@Transactional
class UserService {
    Clock clock

    User createUser(String firstName, String middleName, String lastName, String email, String password) {
        if (User.countByEmail(email)) {
            throw new CreateUserException("EMAIL_IN_USE")
        }

        def salt = BCrypt.gensalt(12)
        def encodedPassword = BCrypt.hashpw(password, salt)
        def user = new User(email: email, password: encodedPassword, registrationDate: Instant.now(clock))
        def person = new Person(firstName: firstName, middleName: middleName, lastName: lastName)
        person.save()
        user.person = person
        user.save()
    }

    User loginUser(String email, String password) {
        def user = User.findByEmail(email)
        if (!user) {
            throw new LoginUserException("EMAIL_NOT_FOUND")
        }
        if (!BCrypt.checkpw(password, user.password)) {
            throw new LoginUserException("PASSWORD_INCORRECT")
        }

        user.loginDate = Instant.now(clock)
        user.save()
    }
}

@InheritConstructors
class CreateUserException extends RuntimeException { }

@InheritConstructors
class LoginUserException extends RuntimeException { }

controllers/UserController.groovy

@RequestMapping("/api/user")
class UserController {
    UserService userService

    @RequestMapping("/create_user")
    def @ResponseBody createUser(@RequestBody CreateUserMessage createUserMessage) {
        try {
            userService.createUser(*createUserMessage.values())
            def builder = new JsonBuilder()
            builder {
                success true
            }
        } catch (CreateUserException ex) {
            def builder = new JsonBuilder()
            builder {
                success false
                message ex.message
            }
        }
    }

    @RequestMapping("/login_user")
    def @ResponseBody loginUser(@RequestBody LoginUserMessage loginUserMessage) {
        if (session.user) {
            def builder = new JsonBuilder()
            return builder {
                success false
                message "ALREADY_LOGGED_IN"
            }
        }

        try {
            def user = userService.loginUser(*loginUserMessage.values())
            session.user = user
            session.token = UUID.randomUUID().toString()
            def builder = new JsonBuilder()
            builder {
                success true
                token session.token
            }
        } catch (LoginUserException ex) {
            def builder = new JsonBuilder()
            builder {
                success false
                message ex.message
            }
        }
    }

    @RequestMapping("/logout_user")
    def @ResponseBody logoutUser(@RequestBody Object object) {
        if (!session.user) {
            def builder = new JsonBuilder()
            return builder {
                success false
                message "NOT_LOGGED_IN"
            }
        }

        session.user = null
        session.token = null
        def builder = new JsonBuilder()
        builder {
            success true
        }
    }
}

@TupleConstructor
class CreateUserMessage {
    String firstName
    String middleName
    String lastName
    String email
    String password

    List values() {
        return [firstName, middleName, lastName, email, password]
    }
}

@TupleConstructor
class LoginUserMessage {
    String email
    String password

    List values() {
        return [email, password]
    }
}

As you can see I'm also injecting a Clock instance as bean in order to facilitate running the server on a different timezone if needed and to facilitate unit testing dates.

The repository can be found here. It also includes lots of unit tests, but I'm not posting those up for review right now, I might do it at a later point in time or if someone makes a special request for it.

\$\endgroup\$
6
\$\begingroup\$

Disclaimer: I don't know sqat about Grails

createUser

I have a few concerns in this method.

Using User.countByEmail seems inefficient. I don't know Grails, but the name of this function suggests that it will scan all the users in the database, when it should stop after the first match, effectively doing an EXISTS query.

The string parameter in all the exceptions that you throw look like technical tags, not like user friendly messages, for example:

throw new CreateUserException("EMAIL_IN_USE")

If they are technical tags that they will be used by another process, then to avoid potentially inconsistent uses, it would be better to convert these strings to constants, and refer to them by the constants.

Lastly, this looks like it should be executed in a transaction:

    person.save()
    user.person = person
    user.save()

That is, if person is saved but an error occurs while saving user, your backend will be in an inconsistent state.

REST API

Some of the REST endpoints are unconventional:

  • /api/user : this is good for GET requests, to get all users
  • /create_user : the common practice is the same endpoint as the one for getting all users, but with POST only
  • /login_user : not too bad, but the qualifier "user" seems redundant: who else would ever login if not a user?

Unit tests

I look forward for your unit tests, in a next question ;-)

\$\endgroup\$
  • \$\begingroup\$ 1) You're right about the "constant" usage, it's in essence a sort of preparation for internationalization. 2) All methods inside a controller annotated with @Transactional are by default run inside a transaction, so we're save there. 3) Good points about the REST names and method names. 4) Unit tests: I'll deliver soonish, but I'm not quite sure on what format to use to post them. \$\endgroup\$ – skiwi Jul 4 '15 at 13:49
1
\$\begingroup\$
  • Regarding the constraints, it seems that you want to make everything nullable and non-nullable fields are the exceptions. There are settings for changing the default values. That can clean up your constraints.
  • Methods with five parameters. I have seen that become a mess. I would suggest to have a separate class to represent that data.
  • Services are Transactional by default so the annotation is redundant.
  • What would happen if a validation error occurs? Unless the database constraints fail you won't get an exception because by default failOnerror is false. I understand that in the particular case you have don't need to worry but it would be better to check whether there were any errors. All domain's have an Errors object. You can use that.
  • Using exceptions as part of your application flow - They hit performance and should be used for exceptional conditions. For validations failure there can be other ways. One way that I can suggest is to have a generic class Domain<T> that contains your domain and an Errors object. From every creation/updation you can return this generic class. The benefit would be internationalization support and not hard-coding everything in your service classes.
  • Generating the encoded password should be done in another service
  • Instead of catching exceptions everywhere you should use Declarative Controller Exception Handling. Read the docs for this section.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.