7
\$\begingroup\$

I'm currently having a bit of code, which I just know can be improved a lot. I am just blind to it. All of my code looks quite neat to me, except these parts... The login + authentication process is taken just way too long because too many calls are being made to the database.

Code is written in Kotlin but syntax is almost the same as Java. (Kotlin is a JVM language, capable of running Java).

Some explanation: I have 3 accounts

  • Plain email accounts - will get a random UUID on succesfull authentication --> used as API key
  • Social media accounts - On first call to API sends an accessToken --> returns UID from Firebase --> used as API key. The API key is stored as UID in the database and is returned to the user. This UID will be validated on each request.
  • LDAP accounts - could send a device_id (if logged in from mobile device) --> generates random UUID --> used as API key. The random UUID gets stored in the database as UID and will be validated on each request

edit There are 2 tables, Participant and Employee. They both have a column called uid or api_key which is quite inconsistent but they're the exact same. This uid or api_key will either be random generated (email or LDAP) or will be retrieved from Firebase by validating the accessToken.

Both the participant and the employee can register for an event and need to be authenticated accordingly.

Only an Employee can use the LDAP login functionality, and only a Participant can use the email account or social media account.

UID or 'api_key' will be returned to the mobile device/browser and will be authenticated by the REST API.


Which can all register for an event.

The relevant Spring REST endpoints which bind the request to a Service' method:

/**
 *@returns UID or empty response --> if no account | registers the participant no matter what
 */
@RequestMapping(value="/events/{eventId}/participants", method=arrayOf(RequestMethod.POST))
fun registerParticipant(@PathVariable eventId: Int,
                        @RequestBody participant: Participant,
                        @RequestHeader(value="media") registeredWith: String,
                        @RequestHeader(value="API_KEY", required=false) uid: String?, // participant with account
                        @RequestHeader(value="Access-Token", required=false) accessToken: String? // participant registered with social media
                        ): Response{
    if(uid != null){
        return ServiceProvider.participantService.registerParticipant(eventId, registeredWith, uid)
    }else{
        if(accessToken != null){
            return ServiceProvider.participantService.registerWithAccessToken(participant, accessToken, eventId, registeredWith)
        } else {
            return ServiceProvider.participantService.registerWithoutUID(participant, eventId, registeredWith)
        }
    }
}

/* ----- POST ----- */

@RequestMapping(value="/participants", method=arrayOf(RequestMethod.POST))
fun addParticipant(@RequestHeader(value="Access-Token", required=false) accessToken: String?,
                   @RequestBody participant: Participant): Response {
    val response = Response()
    if(accessToken != null && accessToken != "") {
        val uid = ServiceProvider.participantService.addParticipantWithAccessToken(participant, accessToken).data
        if(uid != null){
            response.data = uid
        } else {
            response.error = "Something went wrong. Please try again later."
        }
    }else {
        // add a participant without an UID
        val newId = ServiceProvider.participantService.addParticipant(participant, null)
        if(newId > 0){
            response.message = "Registration success"
        } else {
            response.error = "Something went wrong. Please try again later."
        }
    }
    return response
}

The code making all the database calls

The methods:

  • participantExists(): boolean
  • registerForEvent(): boolean
  • hasAccount(): boolean
  • registerForEvent(): boolean
  • addParticipant(): Int (part.id)
  • updateUID(): void
  • participantRegistered(): boolean

Are not shown (else this post would be way too long) but all of the above will call the dao once, so once call every time such a method is executed. Everything instantly calling the dao will also result in a call to the database.

fun registerParticipant(eventId: Int, registeredWith: String, uid: String): Response {
    // validateUID -- returns user
    val participant = validateUID(uid)
    val response = Response()
    if(participant is Participant){
        if(participant.email != ""){
            response.data = participant
            // if user already exists -> only register, else add and register
            val id: Int = participantExists(participant)
            if(id > 0) {
                if(participantRegistered(participant, eventId)) {
                    response.message = alreadyRegisteredMessage
                } else {
                    updateUID(participant, uid, registeredWith, eventId)
                    registerForEvent(participant, eventId, registeredWith)
                    response.message = registerSuccessMessage
                }
            } else {
                // participant doesn't exist --> insert including new UID from the token
                addParticipant(participant, uid)
                registerForEvent(participant, eventId, registeredWith)
                response.message = registerSuccessMessage   
            }
        } else {
            response.error = errorMessage
        }

    }else {
        response.error = errorMessage
    }
    return response
}

 private fun registerIfExists(participant: Participant, eventId: Int, registeredWith: String, id: Int): Response {
    var response = Response()
    participant.id = id
    // check if participant isn't already registered
    if(participantRegistered(participant, eventId)){
        response.message = "U staat al ingeschreven voor dit evenement!"
        return response
    }else {
        // boolean to check if insertion went well
        if(registerForEvent(participant, eventId, registeredWith)){
            // update data in case participant moved/changed phone/wrongly entered previously
            updateParticipant(participant)
            // TODO RETURN UID - if exists
            response.message = "U heeft zich succesvol ingeschreven voor dit evenement."
            return response
        }
        else {
            // throw 400
            response.message = "Er is iets misgegaan, probeer het later opnieuw"
            return response
        }
    }
}

fun registerWithoutUID(participant: Participant, eventId: Int, registeredWith: String): Response {
    var response = Response()
    // check if participant exists
    // (user registered for an earlier event without making account)
    val id: Int = participantExists(participant)
    if(id > 0){
        response = registerIfExists(participant, eventId, registeredWith, id)
        if(hasAccount(participant)){
            response.data = EncryptionUtils.getRandomKey()
            updateUID(participant, response.data.toString(), registeredWith, eventId)
        }
        return response
    }
    // participant doesn't exist, insert and register participant. (without pass & uid)
    else {
        participant.id = addParticipant(participant, null)
        if(registerForEvent(participant, eventId, registeredWith)){
            response.message = registerSuccessMessage
            return response
        }
        else {
            response.message = errorMessage
            return response
        }
    }       
}


fun addParticipantWithAccessToken(part: Participant, accessToken: String): Response {
    var response = Response()
    val token = FirebaseUtils.validateToken(accessToken)
    // token is valid and belongs to this participant
    if(token != null && token.email == part.email && token.email != ""){
        // check if the uid is still valid and if the participant has registered before
        val participant = dao.validateUID(token.uid)
        if(participant != null && participant.email === part.email){
            // participant registered before, return correct `uid`
            response.data = token.uid
        } else {
            // add the participant and return the `uid`
            addParticipant(part, token.uid)
            response.data = token.uid
        }
    } else {
        println("token not valid")
        response.error = "token not valid"
    }
    return response
}
\$\endgroup\$
  • \$\begingroup\$ Are those 3 accounts using different database tables to track the user accounts? And do you have a singular login post request? Why not use a session object that records the UUID, including other data like the user type, expiration, IP Address, etc? \$\endgroup\$ – Phil M Dec 21 '16 at 23:32
  • \$\begingroup\$ edited the question (between the lines above the code). A session object holding the UID is stored but has to be validated on every request. \$\endgroup\$ – Ivaro18 Dec 22 '16 at 8:37
  • \$\begingroup\$ I still think that you should use a single database table for session related data, and track the type of user there. That way there is only a single query for session validation (based on that UID). If there is a chance of collision between uid and api_key, use two different columns. Registering for an event is a different process, but the registration could use much of the data that is already stored in the session data -- basically simplifying everything down to only 2 statements (read for session, and an INSERT ON DUPLICATE IGNORE for registration). \$\endgroup\$ – Phil M Dec 22 '16 at 16:40
  • \$\begingroup\$ Ok but having a single database table for session related data is more of a design flaw than a way to reduce my calls to the database? I already know if the attempted login/registration is a Participant or an Employee based on what login method they use (LDAP (no email) for employees and mail + accessToken for participants). Only the uid is stored, so basically the calls and flow would remain the same. \$\endgroup\$ – Ivaro18 Dec 23 '16 at 7:44
3
\$\begingroup\$

I noticed something with this if statement that bothered me.

  if(uid != null){
      return ServiceProvider.participantService.registerParticipant(eventId, registeredWith, uid)
  }else{
      if(accessToken != null){
          return ServiceProvider.participantService.registerWithAccessToken(participant, accessToken, eventId, registeredWith)
      } else {
          return ServiceProvider.participantService.registerWithoutUID(participant, eventId, registeredWith)
      }
  }

you are placing another if statement directly inside of an else statement and increasing your indentation more than necessary, you should be using an else if instead, like this:

if (uid != null) {
    return ServiceProvider.participantService.registerParticipant(eventId, registeredWith, uid)
} else if(accessToken != null) {
    return ServiceProvider.participantService.registerWithAccessToken(participant, accessToken, eventId, registeredWith)
} else {
    return ServiceProvider.participantService.registerWithoutUID(participant, eventId, registeredWith)
}

I noticed another if statement that looks overly complex

if(participant is Participant){
    if(participant.email != ""){
        response.data = participant
        // if user already exists -> only register, else add and register
        val id: Int = participantExists(participant)
        if(id > 0) {
            if(participantRegistered(participant, eventId)) {
                response.message = alreadyRegisteredMessage
            } else {
                updateUID(participant, uid, registeredWith, eventId)
                registerForEvent(participant, eventId, registeredWith)
                response.message = registerSuccessMessage
            }
        } else {
            // participant doesn't exist --> insert including new UID from the token
            addParticipant(participant, uid)
            registerForEvent(participant, eventId, registeredWith)
            response.message = registerSuccessMessage   
        }
    } else {
        response.error = errorMessage
    }

}else {
    response.error = errorMessage
}
return response

your two outer if statements have an else statement that is exactly the same, this is very redundant and they can be merged together to prevent another level of indentation and repeating yourself.

I would merge the if statements with an And(&&) and have one single else statement, like this

if(participant is Participant && participant.email != ""){
    response.data = participant
    // if user already exists -> only register, else add and register
    val id: Int = participantExists(participant)
    if(id > 0) {
        if(participantRegistered(participant, eventId)) {
            response.message = alreadyRegisteredMessage
        } else {
            updateUID(participant, uid, registeredWith, eventId)
            registerForEvent(participant, eventId, registeredWith)
            response.message = registerSuccessMessage
        }
    } else {
        // participant doesn't exist --> insert including new UID from the token
        addParticipant(participant, uid)
        registerForEvent(participant, eventId, registeredWith)
        response.message = registerSuccessMessage   
    }
}else {
    response.error = errorMessage
}
return response

I found another instance of the first thing I mentioned

if(participantRegistered(participant, eventId)){
    response.message = "U staat al ingeschreven voor dit evenement!"
    return response
}else {
    // boolean to check if insertion went well
    if(registerForEvent(participant, eventId, registeredWith)){
        // update data in case participant moved/changed phone/wrongly entered previously
        updateParticipant(participant)
        // TODO RETURN UID - if exists
        response.message = "U heeft zich succesvol ingeschreven voor dit evenement."
        return response
    }
    else {
        // throw 400
        response.message = "Er is iets misgegaan, probeer het later opnieuw"
        return response
    }
}

in this instance you can also minimize the indentation and use an else if instead of an if inside of the else.

if (participantRegistered(participant, eventId)) {
    response.message = "U staat al ingeschreven voor dit evenement!"
    return response
} else if(registerForEvent(participant, eventId, registeredWith)) {// boolean to check if insertion went well
    // update data in case participant moved/changed phone/wrongly entered previously
    updateParticipant(participant)
    // TODO RETURN UID - if exists
    response.message = "U heeft zich succesvol ingeschreven voor dit evenement."
    return response
} else {
    // throw 400
    response.message = "Er is iets misgegaan, probeer het later opnieuw"
    return response
}
\$\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.