5
\$\begingroup\$

I have a function written in Groovy which needs to save a record and also return response from the server. Function is working fine, but I am not happy with the design because I have a problem with multiple nesting.

This is the function:

Map scanAccessRecord(GrailsParameterMap params){

    Map response
    Date currentTime = new Date()
    String readerEthernetAddress = params.readerMac
    Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress)
    Boolean isAccessIn = (params.isAccessIn == "True")

    if(reader)
    {
        UserSettings userSettings = UserSettings.findByRfidCode(params.rfid.toString())

        if(userSettings)
        {
            if(userSettings.user.company.id == reader.companyId)
            {
                AccessRecord accessRecord = new AccessRecord(reader:reader, userId:userSettings.userId, specificDate:currentTime, isAccessIn: isAccessIn)

                if(accessRecord.save(flush: true))
                {
                    response = [
                            status: '00',
                            message: 'OK',
                            recordId: accessRecord.id,
                    ]
                }
                else
                {
                    response = [
                            status: '60',
                            message: 'error'
                    ]
                }
            }
            else
            {
                response = [status: '40', message: 'USER AND READER ARE NOT FROM SAME COMPANY']
            }
        }
        else
        {
            response = [status: '10', message: 'UNKNOWN USER']
        }
    }
    else
    {
        response = [status: '50', message: 'READER NOT FOUND']
    }

    return response
}

What can I do to make this function "cleaner"?

\$\endgroup\$
9
  • \$\begingroup\$ Not familiar with groovy. Is if(reader) is a null check? \$\endgroup\$
    – shanif
    Commented May 13, 2020 at 0:38
  • \$\begingroup\$ Yes, it is checking whether there is a reader or not \$\endgroup\$
    – Jakov
    Commented May 13, 2020 at 9:31
  • \$\begingroup\$ In what case reader is null? \$\endgroup\$
    – shanif
    Commented May 13, 2020 at 9:42
  • \$\begingroup\$ Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress) Reader is NULL in the case there is no reader found in DB with readerEthernetAddress(mac address) given in params or if it is found but has a flag deleted. \$\endgroup\$
    – Jakov
    Commented May 13, 2020 at 9:48
  • \$\begingroup\$ Is AccessRecord your code or some framework? I Want to understand why save returns boolean. \$\endgroup\$
    – shanif
    Commented May 13, 2020 at 10:07

2 Answers 2

3
\$\begingroup\$
  • Use optional instead of null. I wrote an article about avoiding nulls if you are interested.

  • Use early returns to avoid nesting

    If !reader return [status: '50', message: 'READER NOT FOUND']
    
    If !userSettings return...
    Rest of the logic
    
  • I prefer to avoid nulls,so I would choose to call save with failOnError=true and it will throw exception instead of returning null when save fails.

  • I assume you want any exception to return [ status: '60', message: 'error'], if you use save with failOnError=true then the error is handled in one place only.

  • add validation for params.isAccessIn. If by mistake it is nor true or false, it is better to throw error than do something the client didn't intend to.

  • Maybe you should be more flexible with params.isAccessIn and compare strings without case sensitivity.

\$\endgroup\$
1
  • \$\begingroup\$ Although I am not preferring using early returns in my code, I need to admin that they are very good solution in this case :) \$\endgroup\$
    – Jakov
    Commented May 13, 2020 at 9:43
3
\$\begingroup\$

This is the kind of code I'm having a hard time with myself, so I played around and tried different approaches.

else closer to if condition

The if blocks are far away from their corresponding else blocks. Once you reach the else block you have to go all the way up to lookup what the if condition was. So I would negate all the if conditions. That brings the else closer to the if.

Map scanAccessRecord(GrailsParameterMap params) {
    Date currentTime = new Date()
    String readerEthernetAddress = params.readerMac
    Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress)
    Boolean isAccessIn = (params.isAccessIn == "True")

    if (!reader) {
        [status: '50', message: 'READER NOT FOUND']
    } else {
        UserSettings userSettings = UserSettings.findByRfidCode(params.rfid.toString())
        if (!userSettings) {
            [status: '10', message: 'UNKNOWN USER']
        } else {
            if (userSettings.user.company.id != reader.companyId) {
                [status: '40', message: 'USER AND READER ARE NOT FROM SAME COMPANY']
            } else {
                AccessRecord accessRecord = new AccessRecord(reader: reader, userId: userSettings.userId, specificDate: currentTime, isAccessIn: isAccessIn)
                if (!accessRecord.save(flush: true)) {
                    [status: '60', message: 'error']
                } else {
                    response = [status: '00', message: 'OK', recordId: accessRecord.id]
                }
            }
        }
    }
}

Maybe not beautiful, but I think there's nothing wrong with the code.

Early Returns

Map scanAccessRecordNew(GrailsParameterMap params){
    Date currentTime = new Date()
    String readerEthernetAddress = params.readerMac
    Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress)
    Boolean isAccessIn = (params.isAccessIn == "True")

    if(!reader)
        return [status: '50', message: 'READER NOT FOUND']

    UserSettings userSettings = UserSettings.findByRfidCode(params.rfid.toString())

    if(!userSettings)
        return [status: '10', message: 'UNKNOWN USER']

    if(userSettings.user.company.id != reader.companyId)
        return [status: '40', message: 'USER AND READER ARE NOT FROM SAME COMPANY']

    AccessRecord accessRecord = new AccessRecord(reader:reader, userId:userSettings.userId, specificDate:currentTime, isAccessIn: isAccessIn)

    if(!accessRecord.save(flush: true))
        return  [status: '60', message: 'error']

    return [status: '00', message: 'OK', recordId: accessRecord.id]
}

Is this more readable? Maybe, but I don't like early returns so much either.

Functional Style

Maybe functional programming is the solution. We could use something like Either in Scala. Java's built-in Optional is not well suited when you want to return the reason why something went wrong. Sadly, there is no Either in Standard Java. I have used vavr.io some time ago:

Map scanAccessRecord(GrailsParameterMap params){
    Date currentTime = new Date()
    Boolean isAccessIn = (params.isAccessIn == "True")

    Either reader = findReader(params.readerMac)
    Either userSetting = findUserSetting(params.rfid.toString()).flatMap {
        u -> reader.flatMap { r -> verifyCompanyId(u, r) }
    }

    Either accessRecord = reader.flatMap { r ->
        userSetting.map { u ->
            new AccessRecord(reader:r, userId:u.userId, specificDate:currentTime, isAccessIn: isAccessIn)
        }
    }

    accessRecord.map { saveAccessRecord(it) }.getOrElseGet {it }
}


Either<Map, Reader> findReader(String readerEthernetAddress){
    Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress)
    reader ? Either.right(reader)
           : Either.left([status: '50', message: 'READER NOT FOUND'])
}

Either<Map, UserSettings> findUserSetting(String rfid){
    UserSettings userSettings = UserSettings.findByRfidCode(rfid)
    userSettings ? Either.right(userSettings)
                 : Either.left([status: '10', message: 'UNKNOWN USER'])
}

Either<Map, UserSettings> verifyCompanyId(UserSettings userSettings, Reader reader){
    userSettings.user.company.id == reader.companyId
        ? Either.right(userSettings)
        : Either.left([status: '40', message: 'USER AND READER ARE NOT FROM SAME COMPANY'])
}

Map saveAccessRecord(AccessRecord accessRecord){
    accessRecord.save(flush: true)
        ? [status: '00', message: 'OK', recordId: accessRecord.id]
        : [status: '60', message: 'error']
}

The code in the method scanAccessRecord is somewhat cleaner. But overall the code is longer. And nested flatMaps without syntactic sugar are not easy to read. Does that justify another 3rd party library in your system? Probably not.

Just for Fun

Map-like types are often used for communication between webservices. The pattern:

  • status is 0: OK
  • status is not 0: Error

is very common. So why not use that for something similar to Either? In Groovy we can manipulate the Map class at runtime and add two little methods map and merge calling the method loadMetaUtils() below.

Map scanAccessRecord(GrailsParameterMap params){
    loadMetaUtils()
    Date currentTime = new Date()
    Boolean isAccessIn = (params.isAccessIn == "True")

    Map r = findReader(params.readerMac)
    Map u = findUserSetting(params.rfid.toString())
    u.merge(r).map { verifyCompanyId(userSettings, reader) }
        .map { saveAccessRecord(new AccessRecord(reader:reader, userId:userSettings.userId, specificDate:currentTime, isAccessIn: isAccessIn))}
}


Map findReader(String readerEthernetAddress){
    Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress)
    reader ? [status: '00', reader: reader]
           : [status: '50', message: 'READER NOT FOUND']
}

Map findUserSetting(String rfid){
    UserSettings userSettings = UserSettings.findByRfidCode(rfid)
    userSettings ? [status: '00', userSettings: userSettings]
                 : [status: '10', message: 'UNKNOWN USER']
}

Map verifyCompanyId(UserSettings userSettings, Reader reader){
    userSettings.user.company.id == reader.companyId
            ? [status: '00', userSettings: userSettings, reader: reader]
            : [status: '40', message: 'USER AND READER ARE NOT FROM SAME COMPANY']
}

Map saveAccessRecord(AccessRecord accessRecord){
    accessRecord.save(flush: true)
        ? [status: '00', message: 'OK', recordId: accessRecord.id]
        : [status: '60', message: 'error']
}

void loadMetaUtils() {
    Map.metaClass.map << { Closure f ->
        if(delegate.status != '00') return delegate
        f.setDelegate(delegate)
        f.setResolveStrategy(Closure.DELEGATE_FIRST)
        f()
    }

    Map.metaClass.merge << { Map other ->
        if(delegate.status != '00') return delegate
        if(other.status != '00') return other
        delegate + other
    }
}

You lose all the type saftey, but if you are already dealing with Maps - maybe that's fine. Maybe that's a good way to get your colleagues to like functional programming...

I believe it very much depends on the style used in the code base and the complexity of the method, but in this particular case I would probably go with the first version.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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