4
\$\begingroup\$

I made a commandline tool for renaming files, similar to the rename command in Ubuntu. Here is the code:

import scala.collection.mutable.Map

object Main extends App {
  val usage =
    """
      |A commandline tool for renaming files (written in Scala)
      |
      |Usage:
      |
      |  screname [-a] [-t] -s search_pattern -r replace_pattern filenames
      |
      |Type screname and return to print this message.
    """.stripMargin

  // Print usage and exit
  def printHelpExit() = {
    println(usage)
    System.exit(0)
  }

  if (args.length == 0) {
    printHelpExit()
  }
  val arglist = args.toList
  type OptionMap = Map[Symbol, Any]

  // Is the arg a switch?
  def isSwitch(s: String) = (s(0) == '-')

  // Function for parse args
  def nextOption(map: OptionMap, list: List[String]): OptionMap = {
    list match {
      // List is exhausted
      case Nil => map
      case "-a" :: tail => {
        nextOption(map ++ Map('replaceAll -> true), tail)
      }
      case "-t" :: tail => {
        nextOption(map ++ Map('testRun -> true), tail)
      }
      case "-s" :: searchPattern :: tail => {
        if (map.contains('searchPattern)) throw new IllegalArgumentException("Only one search pattern allowed.")
        nextOption(map ++ Map('searchPattern -> searchPattern), tail)
      }
      case "-r" :: replacePattern :: tail => {
        if (map.contains('replacePattern)) throw new IllegalArgumentException("Only one replace pattern allowed")
        nextOption(map ++ Map('replacePattern -> replacePattern), tail)
      }
      case filename :: tail => {
        map.update('files, filename :: (map.getOrElse('files, List())).asInstanceOf[List[String]])
        nextOption(map, tail)
      }
    }
  }

  val options = nextOption(Map(), arglist)
  // Validate options
  if (!options.contains('searchPattern)) throw new IllegalArgumentException("No search pattern provided")
  if (!options.contains('replacePattern)) throw new IllegalArgumentException("No replace pattern provided")
  if (options.getOrElse('files, List()).asInstanceOf[List[String]].size == 0) throw new IllegalArgumentException("No filenames provided")

  // Rename
  val fileRenamer = new FileRenameByPattern(options.getOrElse('files, List()).asInstanceOf[List[String]], options.getOrElse('searchPattern, "").asInstanceOf[String], options.getOrElse('replacePattern, "").asInstanceOf[String], options.getOrElse('replaceAll, false).asInstanceOf[Boolean])
  if (options.getOrElse('testRun, false).asInstanceOf[Boolean]) {
    fileRenamer.printFileNamePairs()
  } else {
    fileRenamer.rename();
  }
}




import java.io.File

/**
 * Created by IDEA on 17/08/15.
 */

class FileRenameByPattern(val oldFileNames: List[String], val searchPattern: String, val replacePattern: String, val replaceAll: Boolean) {
  val newFileNames = getNewFileNames()

  def getNewFileNames(): List[String] = {
    oldFileNames map {
      (x: String) =>
        if (replaceAll)
          x.replaceAll(searchPattern, replacePattern)
        else
          x.replaceFirst(searchPattern, replacePattern)
    }
  }

  def getFileNamePairs() = {
    oldFileNames zip newFileNames
  }

  def rename(): List[Boolean] = {
    rename(getFileNamePairs())
  }

  def rename(pairs: List[(String, String)]): List[Boolean] = {
    pairs map {
      case (o, n) => new File(o) renameTo new File(n)
    }
  }

  def printFileNamePairs() = {
    val w1 = oldFileNames.map(_.length).max
    val w2 = newFileNames.map(_.length).max
    getFileNamePairs() map {
      case (s1, s2) => println("%%%ds %%%ds".format(w1, w2).format(s1, s2))
    }
  }
}

The -t option is for a dry run.

I am thinking how to make it more extendable, like adding a sequence of numbers at the beginning, appending a date at the end, etc, but any suggestions on improvement are welcome.

\$\endgroup\$
  • \$\begingroup\$ Your option parsing is a bit broken. Asking questions here before putting anything in an answer. 1. "-a" and "-t" can actually appear anywhere (and multiple times) in the arguments. Does that bother you? \$\endgroup\$ – itsbruce Aug 18 '15 at 9:28
  • \$\begingroup\$ 2. While "-s" and "-r" can only appear once, they can appear at any point in the sequence, rather than just in the sequence given by your usage statement. Is any part of the order of arguments important? Or do you not worry as long as they are tin there? \$\endgroup\$ – itsbruce Aug 18 '15 at 9:32
  • \$\begingroup\$ 3. Filenames can appear anywhere in the options sequence. The way your pattern matching is written, screname foo -a bar -t baz -s '.doc' bug -r '.txt' *.* would add "foo", "bar", "baz" and "bug" to the list of files. I'm assuming that's unwanted behaviour but adding it to the question list here for your feedback first. \$\endgroup\$ – itsbruce Aug 18 '15 at 9:40
  • \$\begingroup\$ (1) No, that doesn't really bother me. (2) I think order doesn't matter in this case. (3) I don't mind scattering filenames like that, but I agree this looks a bit messed up. \$\endgroup\$ – qed Aug 18 '15 at 9:54
  • \$\begingroup\$ @itsbruce I think it's probably a good idea to use a args parsing library when I want to add more options. Which library do you recommend? \$\endgroup\$ – qed Aug 18 '15 at 12:05
3
\$\begingroup\$

Option Parsing

As mentioned in comments, your option passing raises some issues.

  1. The parsing does nothing to enforce the sequence given in the usage
  2. Any argument at any position that is not recognised as part of an option is treated as a filename.

This opens the risk of a range of silent errors, which worries me about a utility which I should trust to operate safely on a (potentially large) number of files. There is a simple fix:

  1. Move the filename match to the second slot, just after the Nil match.
  2. Have it throw an exception if the Options map does not already contain 'searchPattern and 'replacePattern

That will eliminate all the worrying errors, leaving only the harmless possibility of multiple '-a' and '-t' options. There are more functional ways to manage the options but that will do enough for this relatively simple requirement.

It seems you've found and adapted the code from the second-highest scoring answer from this SO question. I'm surprised that was voted up so much. It's simple, yes, but also

  • Fragile
  • Requires a lot of code repetition to do even simple things
  • Hard to extend without rewriting most or all of the code
  • Poor example of both style and good practice

(Not that the third point is so important. But... nextOption doesn't return the next option, it returns a map of options - it's badly named to reflect an internal implementation detail. Using list, map and string as variable names is terrible practice, map in particular)

Which library to use

scopt, from the accepted answer in that link, is pretty good. I know the example looks a mess, but it's been made deliberately overcomplicated to show off every single feature. For your code, it would be small, much simpler and more expressive. It groups all the information relevant to an option in one place, including the validation code (and it handles the validation for you, halting as soon as an invalid option is processed, rather than processing them all and then validating them). It provides a clear way to distinguish between options and parameters.

The documentation could be a bit more detailed and it does assume you know which methods are automatically generated for case classes (e.g. copy, which is used everywhere in the example). Note that your config class does not have to be a case class - it just saves you some typing.kj

class FileRanameByPattern

In both rename and printFileNamePairs, why use map? One returns List[Boolean] while the other returns List((), ()) but since you use them both in an if expression, the result is List[Any]. The result is meaningless, so why not use foreach and just return Unit?

\$\endgroup\$

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.