10
\$\begingroup\$

I am trying to send SMS using Twilio with Scala 2.10 in Playframework 2.2, but this code is looking odd to me, maybe due to lots of conditions. I'm sure it needs some important reviews.

val invalidEX = List(21211, 21614, 21608, 30006, 30005, 21604, 21601, 14101)
val reachEX = List(21612, 30004)

def sendSms = Action { implicit request =>
    val smsTo = request.body.asFormUrlEncoded.get("number").head
    val deviceId = request.body.asFormUrlEncoded.get("deviceId").head
    try {
      if(smsTo.length<=6||smsTo.length>=18||deviceId.length>=150){
       Ok(write(Map("result" -> "error")))
      }else{
      if (smsNeeded(smsTo)) {
        if (numberBlocked(smsTo)) {
            if (!todayBlocked(smsTo)) {
              var code = random6Digit
              val appLang =request.acceptLanguages(0).code.toString.splitAt(2)._1
              var body = ""
              appLang.equals("en") match {
                case true =>
                  body = Messages("sms_body", "\n", code)(Lang(appLang))
                case false =>
                  body = Messages("sms_body", "\n", code)(Lang(appLang)) + "\n" + Messages("sms_body", "\n", code)(Lang("en"))
              }
              TwilioAPI.sendSMS(smsTo, body)
              Future {todayEntry(smsTo) }
              Ok(Json.obj("response" -> "success"))
            } else
              Ok(Json.obj("response" -> "temporaryblock"))
        } else
          Ok(Json.obj("response" -> "permanentblocked"))
      } else {
        Ok(Json.obj("response" -> "regular"))
      }
     }
    } catch {
      case twilioEx: TwilioRestException =>
        if (invalidEX.contains(twilioEx.getErrorCode))
          Ok(Json.obj("response" -> "invalidNumber"))
        else if (reachEX.contains(twilioEx.getErrorCode))
          Ok(Json.obj("response" -> "unreachableNumber"))
        else {
          Future { models.LogFile.errorLogs(Json.obj("route" -> request.tags, "post" -> request.body.asFormUrlEncoded.toString, "address" -> request.remoteAddress, "error" -> twilioEx).toString) }
          Ok(Json.obj("response" -> "error"))
        }
      case e: Exception =>
        println(e)
        Future { models.LogFile.errorLogs(Json.obj("route" -> request.tags, "post" -> request.body.asFormUrlEncoded.toString, "address" -> request.remoteAddress, "error" -> e.toString).toString) }
        Ok(Json.obj("response" -> "error"))
    }
  }
\$\endgroup\$
2
\$\begingroup\$

As I'm new to scala, I don't feel I could generate the appropriate code, but I wanted to make a couple points.

First, your method is doing way to much. I see validation, I see messing with a request object, I see sending the SMS, I see response generation. From my point of view, when looking at this method, you can't tell what is going on without a lot of effort. So my recommendation would be to add more methods. If I were writing this, I would have an outer method that abstracts away the HTTP portions of your code

sendSms should pull the string value from the request and then delegate to parseSmsPhoneNumber (or something like that) to perform the logic of validating the string. My second suggestion with this method is that it not return a string, but a type safe object. Now that you have a type safe smsPhoneNumber, you could write another method that does the status checks: is the number blocked?, ... Now that we know everything is good, create a method that takes the smsPhoneNumber and actually sends it. The last bit I would emphasis is that the JSON response generation should happen within sendSms. The other methods will return values types or throw exceptions so signal when bad things happen. Keep all of you HTTP related code isolated to sendSms.

Update:

Below is my take at trying to break your code apart. It is completely untested and there is almost certainly no way that it compiles, but I think it does a reasonable job of demonstrating how you could break things down. So, just think of it as pseudo code that looks similar to scala, but not actually scala.

First, note that I convert things from strings to a type safe type as soon as I possibly can with parseSmsPhoneNumber() and parseSmsDeviceId(). Note that they return an option instead of the actual types. You probably could throw exceptions as well, but I preferred this. The next thing I did was create a bunch of case classes. This is to abstract out the status of situations from happening. There could be a different way to do it, but I liked using case classes because they seem to work with within matches. Next I isolate all of the sending of the sms to withing doSendSms; notice how the exceptions stay within this method. There really isn't any benefit of having the API specific exceptions spill out. If you ever need to change service used to send SMS, then you would only need to modify this one method. Lastly, notice how all of the HTTP related objects (request and response) are isolated to within sendSms. Perhaps you want to do a similar process but without being attached to a request (such as a timed job), with the HTTP information isolated, then this is nearly trivial.

I agree that my pseudo code below still needs more work (i don't like the if/else plus two matches in sendSms, I feel it should be "flatter"), but this is the best I can do right now. Hopefully, some of my points make more sense with some pseudo code.

Completely Un-Tested/UnCompiled Code

val invalidEX = List(21211, 21614, 21608, 30006, 30005, 21604, 21601, 14101)
val reachEX = List(21612, 30004)


case class SmsPhoneNumber(value: String)

def parseSmsPhoneNumber(valueAsString: String): Option[SmsPhoneNumber] = {
  if(valueAsString.length<=6||valueAsString.length>=18) {
    return Some(SmsPhoneNumber(valueAsString))
  } else {
    return None
  }
}


case class SmsDeviceId(value: String)

def parseSmsDeviceId(valueAsString: String): Option[SmsDeviceId] = {
  if(valueAsString.length<=8||valueAsString.length>=150) {
    return Some(SmsDeviceId(valueAsString))
  } else {
    return None
  }
}

sealed trait SmsPhoneNumberStatus
case class SmsNotNeeded extends SmsPhoneNumberStatus
case class NumberBlocked extends SmsPhoneNumberStatus
case class NumberTemporarilyBlocked extends SmsPhoneNumberStatus
case class Valid extends SmsPhoneNumberStatus

def checkSmsPhoneNumberStatus(smsPhoneNumber:SmsPhoneNumber) :SmsPhoneNumberStatus = {
  if (!smsNeeded(smsPhoneNumber)) {
    return SmsNotNeeded
  } else if (!numberBlocked(smsPhoneNumber)) {
    return NumberBlocked
  } else if (todayBlocked(smsPhoneNumber)) {
    return NumberTemporarilyBlocked
  } else {
    return Valid
  }
}

def buildMessageBody(code:String,appLang:String):String = {
  if (appLang.equals("en"))  {
    return Messages("sms_body", "\n", code)(Lang(appLang))
  } else {
    return Messages("sms_body", "\n", code)(Lang(appLang)) + "\n" + Messages("sms_body", "\n", code)(Lang("en"))
  }
}


sealed trait SentSmsResult
case class InvalidNumber extends SentSmsResult
case class UnreachableNumber extends SentSmsResult
case class Error(message:String) extends SentSmsResult
case class Success extends SentSmsResult

def doSendSms(recipient:SmsPhoneNumber,body:String):SentSmsResult = {
  try{
    TwilioAPI.sendSMS(recipient, body)
  } catch {
      case twilioEx: TwilioRestException =>
        if (invalidEX.contains(twilioEx.getErrorCode))
          return InvalidNumber
        else if (reachEX.contains(twilioEx.getErrorCode))
          return UnreachableNumber
        else {
          return Error(twilioEx)
        }
      case e: Exception =>
          return Error(e.toString)
    }
}

def sendSms = Action { implicit request =>
    val smsPhoneNumber = parseSmsPhoneNumber(request.body.asFormUrlEncoded.get("number").head)
    val smsDeviceId = parseSmsDeviceId(deviceId)//not sure where devideId came from

    if(smsPhoneNumber.isEmpty() || smsDeviceId.isEmpty()) {
      Ok(write(Map("result" -> "error")))
    } else {
      checkSmsPhoneNumberStatus(smsPhoneNumber.get) match {
        case SmsNotNeeded => Ok(Json.obj("response" -> "regular"))
        case NumberBlocked => Ok(Json.obj("response" -> "permanentblocked"))
        case NumberTemporarilyBlocked => Ok(Json.obj("response" -> "temporaryblock"))
        case Valid => {
          doSendSms(smsPhoneNumber,buildMessageBody(random6Digit,request.acceptLanguages(0).code.toString.splitAt(2)._1 )) match {
            case InvalidNumber =>  {
               Ok(Json.obj("response" -> "invalidNumber"))
            }
            case UnreachableNumber =>  {
               Ok(Json.obj("response" -> "unreachableNumber"))
            }
            case e:Error =>  {
              Future { models.LogFile.errorLogs(Json.obj("route" -> request.tags, "post" -> request.body.asFormUrlEncoded.toString, "address" -> request.remoteAddress, "error" -> e.message).toString) }
              Ok(Json.obj("response" -> "error"))
            }
            case Success =>  {
              Future {todayEntry(smsTo) }
              Ok(Json.obj("response" -> "success"))
            }
          }
        }
      }
    }
  }
\$\endgroup\$
1
\$\begingroup\$

I have noticed some more problems in my code regarding to performance that

1).

val invalidEX = List(21211, 21614, 21608, 30006, 30005, 21604, 21601, 14101)
val reachEX = List(21612, 30004)

List could be Replace by Vector

val invalidEX = Vector(21211, 21614, 21608, 30006, 30005, 21604, 21601, 14101)
val reachEX = Vector(21612, 30004)

2).

splitAt(2)._1 is looking odd in the code.

val appLang =request.acceptLanguages(0).code.toString.splitAt(2)._1

can be replaced by

val appLang =request.acceptLanguages(0).code.toString.take(2)

3). this validation is looks odd and not enough

if(smsTo.length<=6||smsTo.length>=18||deviceId.length>=150){

need regex or more validation for valid mobile number and deviceId


4).

Ok(write(Map("result" -> "error")))

is same as

Ok(Json.obj("result" -> "error")))

so used one thing Json.obj is preferable because write uses Lift dependecies.


5).

var body = ""
appLang.equals("en") match {
      case true =>
          body = Messages("sms_body", "\n", code)(Lang(appLang))
      case false =>
          body = Messages("sms_body", "\n", code)(Lang(appLang)) + "\n" + Messages("sms_body", "\n", code)(Lang("en"))
 }

could be

var body = Messages("sms_body", "\n", code)(Lang(appLang))
if(!appLang.equals("en"))
          body = body + "\n" + Messages("sms_body", "\n", code)(Lang("en"))
}

6).

The biggest problem that thread is block while waiting for TwilioAPI.sendSMS(smsTo, body) which is approx 3 to 5 seconds. It should be in Future.

\$\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.