Taking some JSON data, converting the date format and calculating a user average

Question

I'm originally a Java developer taking a stab at some Scala development. One of the main reasons I'm posting is because I know that in Scala you can write your code in a Java (OOP) way and it will run but that makes it bad Scala code.

I was hoping I could get a review of my code and maybe somebody could point out how I could be doing things a bit more in a functional way.

A method to get a set of JSON objects from a directory:

def createJsonSessionSet(filePaths: Set[String]) : Set[JSONObject] = {
    val sessions: Set[JSONObject] = Set()
    val parser: JSONParser = new JSONParser

    for (filePath <- filePaths) {
      val jsonObject: JSONObject = parser.parse(new FileReader(filePath)).asInstanceOf[JSONObject]
      sessions.add(jsonObject)
    }

    return sessions
}

A method that lists directories from a file path:

def getListOfJsonFilePaths(file: File): Set[String] = {
    val filePaths: Set[String] = Set()

    for (file <- file.listFiles) {
      filePaths.add(file.getAbsolutePath)
    }

    return filePaths
}

A method that takes a set of json objects and converts the time:

def convertTimeZone(sessions: Set[JSONObject]): Set[JSONObject] = {

val convertedSessions: Set[JSONObject] = Set()
val parser: JSONParser = new JSONParser

val utcFormat: DateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
utcFormat.setTimeZone(TimeZone.getTimeZone("UTC"))

val newFormat: DateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
newFormat.setTimeZone(TimeZone.getTimeZone("Etc/GMT+7"))

for (session <- sessions) {

  val date: Date = utcFormat.parse(session.get("UTCStartTime").toString)

  implicit val formats = Serialization.formats(NoTypeHints)

  val convertedSession = Map(
    "SessionId" -> session.get("SessionId").toString,
    "UserId" -> session.get("UserId").toString,
    "Score" -> session.get("Score").toString,
    "ETCStartTime" -> newFormat.format(date)
  )

  val jsonObject: JSONObject = parser.parse(write(convertedSession)).asInstanceOf[JSONObject]

  convertedSessions.add(jsonObject)

}
return convertedSessions
}

A method that returns a set of JSON objects based on a set of JSON objects:

def getOutputData(convertedSessions: Set[JSONObject]): Set[JSONObject] = {

val outputData: Set[JSONObject] = Set()

val distinctUserIds: Set[String] = Set()
val distinctDays: Set[String] = Set()

for (session <- convertedSessions) {
  distinctUserIds.add(session.get("UserId").toString)
  distinctDays.add(session.get("ETCStartTime").toString.split("T")(0))
}

for (day <- distinctDays) {
  for (userId <- distinctUserIds) {

    implicit val formats = Serialization.formats(NoTypeHints)

    val data = Map(
      "UserId" -> userId,
      "Day" -> day,
      "AverageScore" -> 10
    )
    val parser: JSONParser = new JSONParser

    val jsonObject: JSONObject = parser.parse(write(data)).asInstanceOf[JSONObject]

    if (!jsonObject.get("AverageScore").toString.contains("NaN"))
       outputData.add(jsonObject)

  }
}

return outputData
}

I went about this mini project by first writing it in Java which was easy enough. I had trouble with the next method that returned fine in Java, but not so well in Scala. I think it has something to do with the fact that in Java I used ArrayLists but Scala not supporting that I used sets.

def getUserDailyAverageScore(userId: String, day: String, sessions:  Set[JSONObject]): Double = {
    var sum: Double = 0
    var sessionSize: Int = 0



    for (session <- sessions) {
      println("test")
      if (session.get("UserId").toString.equals(userId) && session.get("ETCStartTime").toString.startsWith(day)) {
        sum += session.get("Score").asInstanceOf[Double]
        sessionSize += 1
      }
    }

    return sum / sessionSize
}

From debugging, the code never steps into the if statement whilst in Java it does.

Answer 1

This answer is by no means complete but I would like to give you a few hints.

Use for-loops sparingly, prefer combinators - map, flatMap, filter, zip etc.

For example in the last snippet, instead of iterating over sessions using for loop, try something like:

val filtered = session.filter(session.get()...)
val sessionSize = filtered.size
val sum = filtered.map(_.get("Score").asInstanceOf[Double]).sum

Always think twice before writing for-loop, it is very likely you can achieve the same using combinators.

Of course, common sense applies, don't try to pack all your code into one long line of combinators - split it into many lines and use indentation and local variables to keep it readable.

You don't have to declare types when defining variables

It is quite typical in Scala code not to declare types for local variables, so try:

val date = utcFormat.parse(session.get("UTCStartTime").toString)

without declaring type :Date

However, if variable has wider scope or providing type would improve readability then, of course, you can still do it. I encourage you to declare return types on public methods.

Learn about different implementations of list in Scala

Most popular ones below.

Mutable:

• ListBuffer - quick sequential access
• ArrayBuffer - quick random access (like Java's ArrayList you mentioned)

Immutable:

• List - implemented as linked list

Prefer immutable ones if you want to code in FP style. Read about "structural sharing" if you have doubts about performance of immutable data structures

Don't use return keyword - it is redundant and potentially dangerous

In your examples you can remove all occurrences of return, read more about usage of return here: Don't use return in Scala

Bonus

Here is a talk from Scala's author where he presents his definition of idiomatic Scala - Scala with Style

Hope this will help.

Answer 2

I want to list some of my suggestions below. I will do it chronologically as you posted your code and try to explain in comments, what bothered me.

def createJsonSessionSet(filePaths: Set[String]) : Set[JSONObject] = {
    val parser = new JSONParser() 
    filePaths.map(path => 
        // you might consider using scala.io instead of FileReader
        parser.parse(new FileReader(path)).asInstanceOf[JSONObject]
    ).toSet
}

1. It is bad style to use collections as mutable ones. Furthermore there is no add method in immutable collections as in Java, adding an element produces a new immutable collection.

2. Omitting parenthesis in Scala is OK, but by convention, you use them whenever the function call is not idempotent. An example for an idempotent function call is List.length. Because List is an immutable data structure a call to length will always yield the same result.

3. Try to use map. It takes a function, that describes how to transform an entry and yields a new collection with those transformed elements.

The final toSet call might not be necessary, because the collection to map is already a Set.

// this returns a Set and no List
def getJsonFilePaths(file: File): Set[String] = {
  file.listFiles.map(_.getAbsolutePath).toSet
}

If you insist to use the resulting collection in the function name, you should make it match.

def convertTimeZone(sessions: Set[JSONObject]): Set[JSONObject] = {
  val parser: JSONParser = new JSONParser

  val utcFormat: DateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
  utcFormat.setTimeZone(TimeZone.getTimeZone("UTC"))

  val newFormat: DateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
  newFormat.setTimeZone(TimeZone.getTimeZone("Etc/GMT+7"))

  sessions.map{ session =>
    val date = utcFormat.parse(session.get("UTCStartTime").toString)
    val convertedSession = Map(
      "SessionId" -> session.get("SessionId").toString,
      "UserId" -> session.get("UserId").toString,
      "Score" -> session.get("Score").toString,
      "ETCStartTime" -> newFormat.format(date)
    )
  }
}

As I stated before, map is your friend.

def getOutputData(convertedSessions: Set[JSONObject]): Set[JSONObject] = {
  // don't create this object every loop step again
  val parser: JSONParser = new JSONParser

  val distinctUserIds = convertedSessions.map(_.get("UserId").toString)
  val distinctDays = convertedSession
    .map(_.get("ETCStartTime").toString.split("T")(0))

  for {
    day <- distinctDays
    userId <- distinctUserIds
  } yield {
    implicit val formats = Serialization.formats(NoTypeHints)
    val data = Map(
      "UserId" -> userId,
      "Day" -> day,
      "AverageScore" -> 10
    )
    parser.parse(write(data)).asInstanceOf[JSONObject]
  }
}

In Scala for-comprehensions can have multiple generators, the compiler will replace them with map and flatMap calls. If you insert yield between the generators and the function block,, the for-comprehension returns a Sequence with the generated elements instead of Unit.

def getUserDailyAverageScore(
  userId: String, 
  day: String, 
  sessions:  Set[JSONObject]):
Double = {
  val userScores = sessions.filter(_.get("UserId").toString.equals(userId))
    .filter(_.get("ETCStartTime").toString.startsWith(day))
    .map(_.get("Score").asInstanceOf[Double]).sum
  userScores.sum / userScores.length
}

It is generally best practice to apply filters first, because the resulting collection will be smaller and therefore iterating over it becomes faster.

My changes are by no means the best way to achieve your goal. There is most likely a faster and more idiomatic approach to it. I only took your code, assuming you are stuck to the used frameworks and tried to make it more idiomatic.

If you have any questions, I will be happy to answer them.