Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

ids contains 100 ids that are comma delimited. Is it fine or does it need improvement?

 def getFriendPoints(ids: String) = {
    DB.withConnection { implicit c =>
      try {
        var pointMap:scala.collection.immutable.Map[String,String]=Map()
        var i=ids.split(",")
        for(pos<-0 to i.size){
        val result = SQL("SELECT  point,privacy FROM `tableName` WHERE id={user}").on("user" -> i(pos)).apply().head
        val point = if (result[Boolean]("privacy")) { result[Double]("point").toString } else { "0" }
        pointMap+=("id"+pos -> i(pos), "point" -> point)
        }
        pointMap
      } catch {
        case e: Exception =>
          Map("result" -> "error")
      }
    }
  }
share|improve this question
    
Am I missing something? You declare pointMap as null then try to call a method on it. How does this ever not return error? –  Daenyth Jun 21 at 15:58
    
Daenyth it was Map() sorry wrong code updated, –  Govind Singh Nagarkoti Jun 24 at 5:36
    
Trying to subvert using the IN clause by using a loop is a pretty bad idea, especially given a solution to use IN: stackoverflow.com/questions/24339011/… –  LimbSoup Jun 25 at 18:13

4 Answers 4

Preamble: I don't know scala. All code you see here is improvised from the question and a few bits of scala that I've seen somewhen. Improvements and comments are welcome

Naming:

I am going to be blunt here... Your names are bad. Not horrible, but seriously bad.

var i = ids.split(",");

first thing jumping me here is, that you have a single thing (the id-string) named with a plural. next thing that jumps me, is that you have a collection with multiple elements and it's name is a single letter.

Rename that stuff:

ids -> idString
i -> idCollection (or even ids, anything is better...)

on we go:

for(pos<-0 to i.size){

usually the iterator in a for loop has your single letter identifier, extremely often, that is our friend i. Also you don't really have to cramop that all together. Give your variables and statements some fresh air:

for (i <- 0 to idCollection.size) {

looks much more readable IMO ;)

val result = SQL([...]

result is a dumb name. result of what? yes, I know it's the SQL statment, but if you just skim it, you could think it's what you want to return in the end.

instead I'd use dbRecord or similar.

Alright, so much for the names, now we get to the real wtf

Storing numbers:

WHY DO YOU STORE NUMBERS AS STRINGS?
AND WHY DO YOU STORE POINTS IN A SEPARATE ENTRY?
pardon caps

Don't shoehorn everything into Strings:

var pointMap:scala.collection.immutable.Map[String,Double]=null
[...]
val point:Double = if (result[Boolean]("privacy")) {
    result[Double]("point")
} else {
    0.0
}
pointMap+=("id" + idCollection(i) -> point)
share|improve this answer

Combine this with andrey's answer about using the IN operator to get back a Stream of results (this is Play framework, right?), then map that to another Stream and convert to a Map. Good scala practice is to use vals not vars.

Also I don't think you were building the map you wanted to build. It looks like you wanted to associate an id with a point value, but I think you ended up with a map with many ("id#" -> "someId") entries and one ("point" -> "123") that got overwritten many times and wasn't associated with any id.

def getFriendPoints(ids: String) = {
  val users=ids.split(",")
  DB.withConnection { implicit c =>
    try {
      val results = SQL("SELECT  point,privacy, id FROM `tableName` WHERE id IN ({users})").onList("users" -> users)
      results().map( result =>
        val id = result[String]("id")
        val point = if (result[Boolean]("privacy")) { result[Double]("point").toString } else { "0" }
        (id -> point)
      ).toMap  //toMap if you want a Map instead of a Stream
    } catch {
      case e: Exception =>
        Map("result" -> "error")
    }
  }
}
share|improve this answer

100 queries is not fine! It's slow. For each select statement there is a significant overhead required to set up and run the query.

Instead you should use a single SELECT statement with a better WHERE clause:

SELECT point, privacy FROM `tablename` WHERE id IN (_set_)

for more information read up on the IN operator

share|improve this answer
    
Wouldn't this method be open to injection though? –  RubberDuck Jun 20 at 21:19

You can use the Try function which is a convenient/compact alternative to writing your useful code inside try/catch (noise) block! The other salient points have been covered by earlier responses, namely using the IN clause in SQL, preferring vals over vars and pattern matching:

import scala.util.Try

def getFriendPoints(ids: String): Map[String, String] = {
  val users = ids.split(",")

  def pointsById(users: Array[String]): Map[String, String] = {
    val rows = SQL("SELECT point, privacy, id FROM `tableName` where id IN ({users}}").onList("users"->users)
    rows.map { row =>
      val id = result[String]("id")
      val point = result[Boolean]("privacy") match {
        case true  => result[Double]("point").toString
        case false => "0"      
      }
      (id -> point)
    }
  }

  DB.withConnection { implicit c =>
    Try(pointsById(users)).getOrElse(Map("result"=>"error"))
  }
}  

Here'a another way, getFriendPoints returns a type Try[Map[String, Double]]. There is not need to make the value type in the Map a String, just to return an error string!:

import scala.util.Try

def getFriendPoints(ids: String): Try[Map[String, Double]] = {
  // contents remain the same


  DB.withConnection { implicit c =>
    Try(pointsById(users)))
  }
}

// Your client code
getFriendPoints(" ... ").getOrElse(Map("Error"->"Message"))
share|improve this answer
    
+1 for pattern matching –  Govind Singh Nagarkoti Jun 23 at 13:48
    
It seems like a good answer based on the feedback of the op, but could you elaborate a bit more ? Why would it be better to use this as opposed to the code of OP ? (You should edit your answer, and not respond to me directly in the comment). –  Marc-Andre Jun 23 at 14:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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