19
\$\begingroup\$

Most of my experience is in Python, Java and embedded C, and I need to learn Scala and Play for a new job. As an exercise, I decided to create a very simple API for looking up area by postal code, backed by Redis. It's just a single controller with a couple of routes.

I'm hoping for feedback from experienced Scala or Play users. Please suggest any improvements to make the code more readable, maintainable, idiomatic, performant, secure etc. As a starting point, I enabled several static analysis tools in my build and fixed the issues reported.

controllers/JPI.scala

package controllers

// Scala imports
import util.control.Breaks._

// Play imports
import play.api.mvc._
import play.api.Play.current
import play.api.cache.Cache
import play.api.libs.json._
import com.typesafe.plugin.RedisPlugin

// Java imports
import java.io.{BufferedReader, InputStreamReader}
import java.net.URL
import java.util.zip.ZipInputStream



object JPI extends Controller {

  final val JapaneseURI = "http://www.post.japanpost.jp/zipcode/dl/kogaki/zip/ken_all.zip"
  final val TestURI = "http://localhost:9000/assets/ken_all.zip"
  final val CSVEncoding = "SJIS"

  final val PostalCodeMin = 0
  final val PostalCodeMax = 9999999
  final val PostalCodeFmt = "%07d"

  final val CSVFields = List(
    ("prefecture_kana", 3),
    ("municipality_kana", 4),
    ("neighbourhood_kana", 5),
    ("prefecture_kanji", 6),
    ("municipality_kanji", 7),
    ("neighbourhood_kanji", 8)
  )

  /**
   Lookup a numeric postal code in the Redis cache
  */
  def lookup(postalCode: Integer) = Action {
    if (postalCode < PostalCodeMin || postalCode > PostalCodeMax) {
      NotFound(
        "Postal code outside valid range (%s to %s)".format(
          fmt(PostalCodeMin), fmt(PostalCodeMax)
        )
      )
    } else {
      val json: Option[String] = Cache.getAs[String](postalCode.toString)
      json match {
        case None => NotFound("Postal code not found")
        case Some(j) => Ok(j).as("application/json; charset=utf-8")
      }
    }
  }

  /**
    Imports postal codes from a zipped CSV reference as follows:
    - Download the complete code directory from the specified URL
    - Unzips the first file and parses the CSV rows
    - Serializes CSV rows into JSON
    - Inserts JSON objects into Redis hash
   */
  def buildCache() = Action {
    val inStream = new ZipInputStream(new URL(TestURI).openStream)
    val csvFile = Option(inStream.getNextEntry)
    csvFile match {
      case Some(f) => {
        val reader = new BufferedReader(new InputStreamReader(inStream, CSVEncoding))
        for (line <- Iterator.continually(Option(reader.readLine)).takeWhile(!_.isEmpty)) {
          breakable {
            val elements = line.getOrElse(break).split(",")
            val code = stripQuotes(elements(2))
            Cache.set(code, Json.stringify(parseCSVRow(elements)))
          }
        }
        Ok("Cache rebuilt")
      }
      case None => InternalServerError("Cache rebuild failed")
    }
  }

  /**
    Format a numeric postal code as a String
  */
  private def fmt(postalCode: Integer) : String = {
    PostalCodeFmt.format(postalCode)
  }

  /**
    Strip trailing and leading quotes from a String
  */
  private def stripQuotes(s : String) : String = {
    s.stripPrefix("\"").stripSuffix("\"")
  }

  /**
    Parse and convert a CSV row into a JSON object according to mapping in CSVFields
  */
  private def parseCSVRow(row : Array[String]) : JsObject = {
    val strippedRow = row map { stripQuotes(_) }
    val fields = CSVFields map {
      f => f._1 -> Json.toJsFieldJsValueWrapper(strippedRow(f._2))
    }
    Json.obj( fields: _* )
  }

}

Building and running this code is as simple as:

git clone https://github.com/bsutherland/jpi.git
cd jpi
sbt compile
sbt run 
\$\endgroup\$
4
\$\begingroup\$

I'm skeptical of accepting postal codes as Integers. Japanese postal codes are of the format NNN-NNNN. Accepting a String, which may contain an optional hyphen and leading zeros, would be more appropriate.

Furthermore, an API that is designed to take an integer could never be extended to countries like the UK and Canada, where postal codes are alphanumeric. It would also be problematic for US ZIP+4 codes, which, despite being mostly numeric like Japanese codes, may overflow an Integer. (It's odd that you take an Integer instead of an Int, but that's a moot point.)

\$\endgroup\$
2
+100
\$\begingroup\$

I know quite little Scala, but even I can understand this code. That's definitely a very good sign!

Postal code validation

I'm wondering if the range check of postal codes in lookup is all that useful. It saves you a cache lookup, but is it really worth it? If you skip that test the method becomes shorter, and also more flexible. If someday they add one more digit, your program will continue to work happily without having to change the validation logic.

I considered that the check is still useful as input validation, but the cache lookup already fulfills that role anyway. The message is also inaccurate: the min and max postal codes as of today are technically 0010000 and 9998531, instead of 0000000 and 9999999. I would drop the check to make the code shorter. Consider that the less code you have the better, and the check doesn't add much value.

Naming

I think the name CSVFieldNameIndexes would be better than CSVFields. Although when I look at the variable assignment, I can guess that it's about field name and index pairs, this becomes less clear when you use the variable to map the fields in parseCSVRow. With this rename, the parseCSVRow method becomes a bit easier to read.

Documentation

Since you want to create an "API", the documentation of public methods is very important.

I'm wondering if it's significant that your service is backed by a Redis cache:

  /**
   Lookup a numeric postal code in the Redis cache
  */
  def lookup(postalCode: Integer) = Action {

I think that's an implementation detail, and clients of your API don't really need to know when using this method. So I would just call it "cache" instead of "Redis cache" specifically.

Error handling

If the ZIP file is empty, the program does this:

  case None => InternalServerError("Cache rebuild failed")

This is a bit too generic, and not very helpful. Although you have to be careful not to divulge too much information in error messages, I think it will be better to make this message a bit more informative, for example "Cache rebuild failed: ZIP file of postal codes is empty".

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