4
\$\begingroup\$

I am building game, like Ogame, or Ikariam where you can play with other users in real-time. You train some soldiers, and build some vehicles or buildings. Every building or vehicle has some level. If a building has a higher level you must wait longer to use it etc. I will be grateful for any comments, especially about Scala language.

import java.text.SimpleDateFormat
import java.util.Date

import com.redis._

object worker extends App {

    override def main(args: Array[String]): Unit = {
        println("Worker start...")
        val r = new RedisClient("localhost", 6379)
        var i: Int = 0

        while (true) {
            Thread.sleep(1000)
            i = i + 1
            val numbers: List[Int] = getUsers(r)
            checkQueue(r, numbers)
        }
    }

    private def checkQueue(r: RedisClient, numbers: List[Int]): Unit = {
        val d1 = new Date()
        val format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss")
        val now = new Date()
        for (nr <- numbers) {
            val ranges = r.hkeys("user_" + nr + ":soldier:queue_time")
            ranges match {
                case Some(ss) => for (range <- ss) {
                    val added_time = r.hget("user_" + nr + ":soldier:queue_time", range)
                    val saved = format.parse(added_time.get)
                    val diff = (now.getTime - saved.getTime) / 1000 // diff in sec
                    val interval = r.hget("user_" + nr + ":soldier:interval", range)
                    val modulo = diff % interval.get.toInt
                    if (modulo == 0) {
                        val queue_amount = r.hget("user_" + nr + ":soldier:queue_amount", range)
                        if (queue_amount.get.toInt >= 1) {
                            r.hincrby("user_" + nr + ":soldier:amount", range, 1)
                            r.hincrby("user_" + nr + ":soldier:queue_amount", range, -1)
                        }
                    }
                }
                case None =>
            }
        }
        val d2 = new Date()
        println("loop time: " + (d2.getTime - d1.getTime) + " milliseconds")
    }

    private def getUsers(r: RedisClient): List[Int] = {
        val list = r.keys("*user_*:soldier:interval*")
        var numbers = List[Int]()

        list match {
            case Some(s) => for (k <- s) {
                val key = k.get
                val nr = key.replaceAll("[^0-9?]", "")
                numbers = nr.toInt :: numbers
            }
            case None =>
        }
        numbers
    }
}
\$\endgroup\$
4
\$\begingroup\$

The first problem with this code is that there is too little explanation for anything. There is no documentation. There is a referenced class RedisClient for which there is no explanation of what it is and how it is being used. The variable names tend to be undescriptive. When asking other people to read code, then it is the author's responsibility to make it so that they actually can read and follow the code.

The next major problem is a classic issue for newer programmers (and something I have been guilty of myself) - big monolithic procedures, specifically checkQueue. When the size of a procedure exceeds a reasonable size (definition of reasonable vary from person to person, but certainly no more than 10 lines), then the developer should look very hard for ways to refactor it and extract out pieces of discrete functionality. In this code, everything inside the for (range <- ss) block should almost certainly be extracted into a function of its own (with a descriptive function name).

I would recommend fixing these two issues and then post a new question asking for a new review of the modified code.

\$\endgroup\$
  • \$\begingroup\$ I especially like the monolithic procedures bullet point. It's a very large problem even with experienced developers. It's easy for us to forget that there are better ways of abstracting things out. (I use C#, and recently I've been using a lot of event-type dealings.) \$\endgroup\$ – Der Kommissar Jul 8 '15 at 19:42

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.