11
\$\begingroup\$

I'm learning Scala and could use another set of eyes on the code below. I'm creating a test runner for the 99 scala problems set. I figure between actually solving the problems and working through the pains of setting up a new IDE (trying IntelliJ from Eclipse), using new build tools (trying SBT from Maven), new testing tools etc, I'll have the language down pat. The code below is a runner that grabs all the solution files in a given directory and executes them. While the code works, I can't help but feel as though I'm doing this in the 'Java' way and not the 'Scala' way (ie - I'm not being very 'functional' in my implementation). Does anyone who knows Scala pretty well have any comment on what I've written thus far?

runner:

import java.io.File
import org.apache.commons.io.FilenameUtils



/**
 * boodstraps program
 */
object bootStrap {

    def main(args: Array[String]): Unit = {

      // go through solution files and execute
      for (fName <- getSolutionFiles(new File("./src/main/scala"))) {
        val fNameNoExtS = FilenameUtils.removeExtension(fName.getName)
        solutionRunner(fNameNoExtS)
      }

    }


    def solutionRunner(name: String): Unit = {
      println(name)
      val classLoader = this.getClass.getClassLoader
      val clazz = classLoader.loadClass(name)
      val solutionMethod = clazz.getDeclaredMethod("solution")
      solutionMethod.invoke(clazz.newInstance)
    }


  /**
   * trolls through working directory and all sub directories in search of files
   * with names starting with "sp_" ('sp' for 'scala problem'). thanks to
   * stack overflow for base code: http://stackoverflow.com/questions/2637643/
   * how-do-i-list-all-files-in-a-subdirectory-in-scala
   *
   * @param f - java File object
   * @return targetFileList - Array[java.io.File] - list of file objects
   *         that have names starting with "sp_"
   */
    def getSolutionFiles(f: File): Array[File] = {
      // create lists of files, directories, and target files
      val fullFileList = f.listFiles
      val targetFileList = fullFileList.filter(_.getName.matches("^sp_.*"))
      val dirList = fullFileList.filter(_.isDirectory)

      // recursively append target files located in sub directories to list.
      // return populated list to caller.
      targetFileList ++ dirList.flatMap(getSolutionFiles)
    }

}

simple 'sp' file:

class sp_01 {

  def solution(): Unit = {
    println("in sp_01, baby!")
  }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ Hi! Welcome to Code Review. Your title should explain the purpose of the code, and not what concerns you have. \$\endgroup\$ – TheCoffeeCup Feb 18 '15 at 20:13
  • \$\begingroup\$ thx and will do in future! \$\endgroup\$ – David Holiday Feb 18 '15 at 20:59
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Feb 23 '15 at 14:54
  • \$\begingroup\$ w00t - t/y everyone for all the great suggestions! I'm posting all this to gitHub (user projectvalis) so hopefully it will help some future scala initiate work out the nuances of the language. \$\endgroup\$ – David Holiday Feb 23 '15 at 16:39
2
\$\begingroup\$

Idiomatic Scala Style

  • Class, object, and trait names follow camelCase style with the first letter capitalized. One example where you don't follow this rule is abstract class spMeta extends logging.

  • For a few reasons, it is considered bad practice to user underscores in names. Applying the convention from the bullet point above with this we'd make the following transformation to your code: class sp_01 extends spMeta => class Sp01 extends SpMeta.

  • The above rules also extend to types that you define. Consequently I'd change type inputT to type InputT, etc.

  • Methods which act as accessors should be declared without parenthesis (unless they have side effects). In particular this applies to your def getSolution(): SolutionT = ... which should just be plain old parenthesis-less def getSolution: SolutionT = ...

  • If you ever have any style questions check out this documentation.

Code Suggestions

A modified version of your getSolutionFiles method with explanations below.

    def getSolutionFiles(dir: File): Array[File] = {
        val dirContents = dir.listFiles
        val targetFiles = dirContents filter(x => x.getName.startsWith("sp_") && !x.isDirectory)
        val childDirs   = dirContents filter(_.isDirectory)
        targetFiles ++ childDirs flatMap(d => getSolutionFiles(d))
    }
  • I renamed the method paramter to dir from file.
  • All of the operations in this method are on the container type Array. So I renamed some of the values to remove any mention of List.
  • Overall I think the renaming of these values make this method much easier to to immediately comprehend.
  • Just to show you 'another way' and not because I thought anything was wrong with your original choice, I replaced your use of a regular expression for finding the correct files with startsWith(...). Note there is also a method endsWith(...).
  • A bug could occur if the name of a file passed your filter predicate but is in fact a directory. I added a condition to to eliminate that possibility.

Cheers :)

\$\endgroup\$
1
\$\begingroup\$

It's easier to do the nitpicks first, so here goes:

/**
 * boodstraps program
 */

this should be bootstraps (which you spelled correctly just one line later)

/**
 * [...] thanks to
 * stack overflow for base code: http://stackoverflow.com/questions/2637643/
 * how-do-i-list-all-files-in-a-subdirectory-in-scala
 * [...]
 */

This is awesome. One of the few times I see credit where credit is due. It is my sorrowful duty to inform you that you didn't fulfil your responsibilities completely, though. You shouldn't thank StackOverflow, but the user who has written the answer:

/**
 * [...] Thanks to user {@link http://stackoverflow.com/users/247533/rex-kerr Rex-Kerr} 
 * for the base code as seen in {@link http://stackoverflow.com/a/2638109/1803692 this SO answer}
 * [...]
 */

or something along these lines ;)

Now for the real beef. I'll just say it right out: I have little to no experience in scala, so I can't say much about the scalaness of the program.

Overall you have a clear separation of responsibilities, and a very straightforward approach to this. But there's some thing's I'd like to touch upon:

  1. Inline Comments:

    // go through solution files and execute
    

    You have the exact same simply by reading the following three lines of code. This comment does not add any value to the code. The same holds true for the next inline comment:

    // create lists of files, directories, and target files
    

    The one after that is better, but that may be the case, because my scala is simply speaking: bad.

  2. Use of java.io.File:
    The api of java.io.File is a little problematic, mostly because Exceptions don't provide all that much information and some other stuff. (more information here). I strongly recommend you instead use java.nio.file.Path which has a cleaner and easier API.

\$\endgroup\$
  • \$\begingroup\$ all good notes - t/y! \$\endgroup\$ – David Holiday Feb 19 '15 at 17:49

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.