I'm learning Kotlin and I made a todo program (in command line) with Kotlin and Maven as my build tool.

Full project.

Description

The idea of my program is to read a file (path defined in a config file) that contains the list of tasks.

Since it's a project for practice purpose, the content of the file is quite simple and looks like this:

foo;true
bar;false
foobar;true

2 columns:

  • Description of the task
  • Status of the task

The only actions I've created are the following:

  • list the tasks (on the standard output)
  • add a new task (by writing a new line on the file)
  • finish a task (by setting the status of the task to true)

There are also other options:

  • help: display the list of options in the standard output
  • config: use another configuration file
  • verbose: display the debug logs
  • version: display the version of the program

The code

I'm using commons CLI library to help me parse command line options passed in the arguments.

I represented a task with the following:

class Task(var name: String, var isDone: Boolean = false) {
    // Override toString to display the task in "human"
    override fun toString(): String {
        var done = ""
        if (isDone) {
            done = "X"
        }
        return "$name \t$done"
    }
    // function to transform the task in a line to be written in the file
    fun toLine(): String {
        return "$name$SEPARATOR$isDone"
    }
}

In order to transform a line from my task file into a Task instance, it's quite straightforward:

fun parse(line: String): Task {
    val array = line.split(SEPARATOR)
    if (array.size < 2) {
        return Task("", false)
    }
    return Task(sanitize(array[0]), sanitize(array[1]).toBoolean())
}

fun sanitize(s: String): String {
    return s.replace("\n", "")
}

I created an interface Cmd which represents a command:

interface Cmd {
    fun getOption(): Option
    fun isEnabled(line: CommandLine): Boolean
    fun getOptionValue(line: CommandLine): String?
}

I also created a ActionCmd which represents an action command, like adding, finishing, or listing the tasks. This ActionCmd implements the Cmd interface:

interface ActionCmd: Cmd {
    fun execute(p: Path, arg: String)
}

In order to avoid duplicating the code for all my commands, I created an abstract class AbstractCmd that uses the commons-CLI classes:

abstract class AbstractCmd: Cmd {
    override fun isEnabled(line: CommandLine): Boolean {
        return line.hasOption(getOption().longOpt)
    }

    override fun getOptionValue(line: CommandLine): String? {
        return line.getOptionValue(getOption().longOpt)
    }
}

Finally, the action commands are the following:

class ListCmd : AbstractCmd(), ActionCmd {
    private val option = Option("l", "list", false, "print the list of tasks")
    override fun getOption(): Option {
        return option
    }

    override fun execute(p: Path, arg: String) {
        val tasks = ArrayList<Task>()
        val stream = Files.newBufferedReader(p)
        stream.buffered().lines().forEach { line -> tasks.add(parse(line)) }
        tasks.forEach { println(it) }
    }
}

class AddCmd: AbstractCmd(), ActionCmd {
    private val option = Option("a", "add", true, "add a task")
    override fun getOption(): Option {
        return option
    }

    override fun execute(p: Path, arg: String) {
        // I have to add the empty string, otherwise, it's not going to add a new line
        p.toFile().appendText("" + Task(arg).toLine())
    }
}

class FinishCmd : AbstractCmd(), ActionCmd {
    private val option = Option("f", "finish", true, "finish a task")
    override fun getOption(): Option {
        return option
    }

    override fun execute(p: Path, arg: String) {
        val task = Task(arg)

        val tasks = ArrayList<Task>()
        val readerStream = Files.newBufferedReader(p)
        readerStream.buffered().lines().forEach { line ->
            val t = parse(line)
            if (t.name == task.name) {
                t.isDone = true
                debug("Finishing task: $t")
            }
            tasks.add(t)
        }
        File(p.toString()).printWriter().use { out ->
            tasks.forEach { out.println(it.toLine()) }
        }
    }
}

Does my code follow common best practices or am I going the wrong way?

Here are some recommendations:

Use if as expression when it's appropriate.

var done = ""
if (isDone) {
    done = "X"
}

The better way is to write:

val done = if (isDone) "X" else ""

Also try to use val as much as it's possible. That will make your code simpler and more reliable.

Don't write obvious comments like comments for toString or toLine functions.

I would rename functions toLine and parse to serialize/deserialize. Or I'd use just toString + parse.

Using global variables like SEPARATOR is a bad idea.

I'd recommend use data class for Task and Commands.

private val option = Option("a", "add", true, "add a task")
override fun getOption(): Option {
    return option
}

It equals to

val option = Option("a", "add", true, "add a task")

Also it's a good practice to avoid using in functions many arguments with the same type. If you can't avoid you still can name them explicitly by writing method(param = arg1..)

This looks weird

p.toFile().appendText("" + Task(arg).toLine())

Try to use more filters, maps, and other hi-order functions over collections

val tasks = ArrayList<Task>()
val stream = Files.newBufferedReader(p)
stream.buffered().lines().forEach { line -> tasks.add(parse(line)) }
tasks.forEach { println(it) }

This is just:

p.toFile().readLines().map{ parse(it) }.forEach {
    println(it)
}

Another example:

    val tasks = ArrayList<Task>()
    val readerStream = Files.newBufferedReader(p)
    readerStream.buffered().lines().forEach { line ->
        val t = parse(line)
        if (t.name == task.name) {
            t.isDone = true
            debug("Finishing task: $t")
        }
        tasks.add(t)
    }

Is just

val tasks = p!!.toFile().readLines().map { parse(it) }
commands.filter { it.name == task.name }.forEach { it.isDone = true }

If you want I can rewrite your program on my taste (as example).

  • "Using global variables like SEPARATOR is a bad idea." I wanted to use a constant, not a global variable. What should be done instead? val option = Option("a", "add", true, "add a task") The getOption method was declared in the interface as I needed to abstract the implentation when fetching all the options. – l-lin Mar 25 at 16:14

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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