patternsqlModerate
Multiple SQL queries, one per ID
Viewed 0 times
persqlonemultiplequeries
Problem
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 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")
}
}
}Solution
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.
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:
usually the iterator in a for loop has your single letter identifier, extremely often, that is our friend
looks much more readable IMO ;)
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
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:
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)Code Snippets
var i = ids.split(",");for(pos<-0 to i.size){for (i <- 0 to idCollection.size) {val result = SQL([...]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)Context
StackExchange Code Review Q#54783, answer score: 10
Revisions (0)
No revisions yet.