A rename utility in Scala

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 pa

Option Parsing

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

• The parsing does nothing to enforce the sequence given in the usage

• 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:

• Move the filename match to the second slot, just after the Nil match.

• 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?

StackExchange Code Review Q#101234, answer score: 3