Scala Docx generator from CSV input, using the docx4j library and Scala Swing

Question

I am working on an application which takes as input a CSV file containing a list of attributes and a .docx template containing variable names equal to the columns of the CSV, and generates one new .docx for each tuple of the CSV, with the variables replaced by the contents of each 'cell'.

The application uses a point-and-click Scala Swing GUI where the end user can provide the files for input, and the destination for the output.

I tried to make the code as clean as possible, but I still would like to know how it can be improved. I personally think the InteractionMediator class is doing too much, and would like to think of ways to delegate some of its tasks to other entities in a clean way.

I would also like to make it more testable. I know about TDD, but have not really used it while creating this. I'm thinking that maybe I should do a rewrite and do TDD this time.

The InteractionMediator class below is the one which coordinates most of the work, so I am posting it here. But if anyone is interested in the source code for all the other entities it can be found on my GitHub repo.

package lettergenerator
package renderer

import formatter._

import org.docx4j.XmlUtils
import org.docx4j.wml.Document
import org.docx4j.jaxb.Context
import org.docx4j.openpackaging.io.SaveToZipFile
import org.docx4j.openpackaging.packages.WordprocessingMLPackage  
import org.docx4j.openpackaging.parts.WordprocessingML.MainDocumentPart

import scala.swing.MainFrame

import java.util.{HashMap => JHashMap}

import scala.annotation.tailrec

import scala.concurrent.Future
import scala.concurrent.ExecutionContext.Implicits.global

case class InteractionMediator() {
  private var gui: Wizard = _ 

  def registerInterface(gui: MainFrame): Unit = this.gui = gui.asInstanceOf[Wizard]

  def hasGui: Boolean = Option[MainFrame](gui) match {
    case Some(_) => true
    case None => false
  }

  def runInterface(): Unit = gui.visible = true

  def messageUser(text: String): Unit = gui.message(text)


  def submit(): Unit = {
    messageUser("Processing...")
    Future { validatePaths(PathValidator()) }
  }


  def validatePaths(validator: Validator ): Unit = {
    val paths = List[(String,String)](
      "details file" -> gui.detailsFile,
      "template file" -> gui.templateFile,
      "destination folder" -> gui.destinationFolder
    )

    val message = "Could not reach the %s. Please check if path is correct"+
      ", or report this issue"

    vldt[String](
        paths,
        validator,
        loadDetails(DetailsFormatter(CsvInput(gui.detailsFile))),
        message)
  }


  def loadDetails(form: DetailsFormatter): Unit = {
    val details: List[Map[String,String]] = form.details

    val detailsMessage = "Details file error: the row with values "+
      "%s is incomplete. Please check it and try again" 

    validateDetails(details,DetailsValidator(form.headers), detailsMessage)
  }


  def validateDetails(details: List[Map[String,String]],
      validator: DetailsValidator, message: String): Unit = {

    var flag = false
    try {
      for (mapElement <- details) 
        vldt[Map[String,String]](
          List((mapElement.values.mkString(" "),mapElement)), 
          validator, 
          flag = true,
          message)
    } catch {
      case e: Throwable => {
        gui.message("Error")
        e.printStackTrace()
        gui.alert(e.getStackTrace.mkString("\n"))
      }
    }
    if(flag) loadTemplate(details)
  }


  def loadTemplate(details: List[Map[String,String]]): Unit = {
    val docPack: WordprocessingMLPackage = 
      TemplateFormatter(DocxInput(gui.templateFile))
        .template

    validateTemplate(details, docPack)
  }


  def validateTemplate(details: List[Map[String,String]], 
      docPack: WordprocessingMLPackage): Unit = {
    val docText: String = WordMLToStringFormatter(docPack).text
    val validator = TemplateValidator(docText)
    val message: String = "Error: could not find variable %s on template."
    val headers: List[(String,String)] = gui.fnAlsoInTemplate match {
      case true => details.head.keySet.map(header => (header,header)).toList
      case false => details.head.keySet.filter(_ != gui.fNameColumn)
        .map(header => (header,header)).toList
    }
    vldt[String](headers,validator,generateLetters(details,docPack),message)
  }


  def generateLetters(details: List[Map[String,String]],
      docPack: WordprocessingMLPackage): Unit = {
    import scala.collection.JavaConverters._

    val destination: String = gui.destinationFolder
    val template: MainDocumentPart = docPack.getMainDocumentPart
    val duplFileChecker = PathValidator()

    @tailrec
    def fileName(name: String, counter: Int): String = {
      val increment = counter + 1
      if (duplFileChecker.validate(destination+"/"+name+".docx")) {
        if (duplFileChecker.validate(destination+"/"+name+increment+".docx")) 
          fileName(name,increment)
        else destination+"/"+name+increment+".docx"
      } else destination+"/"+name+".docx"
    }

    for(smap <- details) {
      val fname = smap.collectFirst({
        case (k: String,v: String) if k == gui.fNameColumn => v
      }) match {
        case Some(file) => file
        case None => "Output"
      }

      val map: JHashMap[String,String] = gui.fnAlsoInTemplate match {
        case true => new JHashMap(smap.asJava)
        case false => new JHashMap(smap.filter(_._1 != gui.fNameColumn).asJava)
      }

      val jaxbElement = template.getJaxbElement
      val xml: String = XmlUtils.marshaltoString(jaxbElement, true)
      val replaced: Object = XmlUtils.unmarshallFromTemplate(xml, map)
      template.setJaxbElement(replaced.asInstanceOf[Document])

      new SaveToZipFile(docPack).save(s"${fileName(fname,0)}")
      template.setJaxbElement(jaxbElement)
    }
    messageUser("Done!")
  }

  @tailrec
  private def vldt[A](p: List[(String,A)], validator: Validator,
      op:  => Unit, message: String): Unit = p match {
    case Nil => 
      throw new IllegalArgumentException("argument p cannot be an empty list")

    case x :: Nil => validator.validate(x._2) match {
      case true => op
      case false => messageUser(message.format(x._1))
    }

    case x :: xs => validator.validate(x._2) match {
      case true => vldt(xs,validator,op,message)
      case false => messageUser(message.format(x._1))
    }
  }

  def columnsForFileName(): List[String] = {
    val path: List[(String,String)] = List(("details file", gui.detailsFile))
    val validator = PathValidator()
    val message = "Could not reach the %s. Please check if path is correct"+
      ", or report this issue"

    var columns = List[String]()

    vldt[String](path, validator, columns = DetailsFormatter(
      CsvInput(path.head._2)).details.head.keySet.toList, message)

    List("") ++ columns
  } 
}

Below is the code for the GUI. This is the second time I use Swing, and the first time I use it in Scala, so I am still finding my bearings with it. So it would be very helpful if anyone could advise on bad practices or things which I could do to make the code cleaner:

package lettergenerator
package renderer

import scala.swing._
import scala.swing.event._

import java.io.File
import javax.swing.filechooser.FileNameExtensionFilter

/**
 * the main frame. Responsible for laying out the elements
 * @param medium an InteractionMediator object
 */
class Wizard(medium: InteractionMediator) extends MainFrame {
  title = "Letter Maker Wizard" 
  preferredSize = new Dimension(695,360)
  val TextWidth = 56

  // for making the buttons, labels and textfields
  val elementMkr = ElementMaker()

  // for opening files and directories
  private val csvOpener = new FileChooser(new File("."))
  csvOpener.fileFilter = (new FileNameExtensionFilter("CSV (Comma Separated Values)","csv"))
  private val docxOpener = new FileChooser(new File("."))
  docxOpener.fileFilter = (new FileNameExtensionFilter("Word Document","docx"))
  private val dirOpener = new FileChooser(new File("."))
  dirOpener.fileSelectionMode = FileChooser.SelectionMode.DirectoriesOnly


  // source of letter header details
  private val (dtLbl, dtTxt, dtBtn) = 
    elementMkr.mkOpenFileElmts("Please choose the file with the"
      + " details which will go on the letters", csvOpener, TextWidth)

  // drop down box for file name column      
  private var textChangeFlag = dtTxt.text
  private val fileNameColumn = new ComboBox(List[String]())
  private val fnLbl = elementMkr.label(" ") 
  def fNameColumn: String = fileNameColumn.selection.item

  // check box to check if file name is also present in template
  // as a variable to be replaced
  private val fnAlsoInTemplate_ = new CheckBox("File name also part of letter")
  fnAlsoInTemplate_.selected = false
  def fnAlsoInTemplate: Boolean = fnAlsoInTemplate_.selected

  // source of letter template
  private val (tpltLbl, tpltTxt, tpltBtn) = 
    elementMkr.mkOpenFileElmts("Please choose the file with the "
      + " template for the letters", docxOpener, TextWidth)

  // destination folder
  private val (destLbl, destTxt, destBtn) = 
    elementMkr.mkOpenFileElmts("Please choose a destination " 
      + "folder for the letters", dirOpener, TextWidth)

  private val msg: Label = elementMkr.label("Ready")

  def message(text: String): Unit = msg.text = text

  def alert(text: String): Unit = Dialog.showMessage(this,text,"Alert")

  listenTo(dtTxt)

  reactions += { case ValueChanged(dtTxt) => comboBoxRoutine() }

  setMaxHeight(dtTxt)
  setMaxHeight(tpltTxt)
  setMaxHeight(destTxt)
  setMaxHeight(fileNameColumn)

  val VShortGap: Int = 5
  val VLargeGap: Int = 30
  val HShortGap: Int = 3

  contents = new BoxPanel(Orientation.Vertical) {
    contents += new BoxPanel(Orientation.Vertical) {
      contents += dtLbl
      contents += Swing.VStrut(VShortGap)
      contents += new BoxPanel(Orientation.Horizontal) {
        contents += dtTxt
        contents += Swing.HStrut(HShortGap)
        contents += dtBtn
      }
      contents += Swing.VStrut(VShortGap)
      contents += new BoxPanel(Orientation.Vertical) {
        contents += fnLbl
        contents += Swing.VStrut(VShortGap)
        contents += new BoxPanel(Orientation.Horizontal) {
          contents += fileNameColumn
          contents += Swing.HStrut(HShortGap)
          contents += fnAlsoInTemplate_
        }
      }
    }

    contents += Swing.VStrut(VLargeGap)
    contents += new BoxPanel(Orientation.Vertical) {
     contents += tpltLbl
     contents += Swing.VStrut(VShortGap)
     contents += new BoxPanel(Orientation.Horizontal) {
       contents += tpltTxt
       contents += Swing.HStrut(HShortGap)
       contents += tpltBtn
     }
    }

    contents += Swing.VStrut(VLargeGap)

    contents += new BoxPanel(Orientation.Vertical) {
     contents += destLbl
     contents += Swing.VStrut(VShortGap)
     contents += new BoxPanel(Orientation.Horizontal) {
       contents += destTxt
       contents += Swing.HStrut(HShortGap)
       contents += destBtn
     }
    }

    contents += Swing.VStrut(VShortGap)

    contents += new BoxPanel(Orientation.Horizontal) {
      contents += elementMkr.button("Generate Letters", submit())
      contents += Swing.HGlue
    }

    contents += Swing.VStrut(VShortGap)

    contents += new BoxPanel(Orientation.Horizontal) {
      contents += msg
      contents += Swing.HGlue
    }

    for (e <- contents)
      e.xLayoutAlignment = 0.0
    border = Swing.EmptyBorder(10, 10, 10, 10)
  }


  def setMaxHeight(comp: Component) = 
    comp.maximumSize = new Dimension(Short.MaxValue, comp.preferredSize.height)

  def submit(): Unit = medium.submit() 

  def detailsFile: String = dtTxt.text
  def templateFile: String = tpltTxt.text
  def destinationFolder: String = destTxt.text

  def comboBoxRoutine(): Unit = {
    if (dtTxt.text != textChangeFlag) {
      fileNameColumn.peer.setModel(
          ComboBox.newConstantModel(
              medium.columnsForFileName()))
      textChangeFlag = dtTxt.text
      fileNameColumn.selection.item = ""

      if (fileNameColumn.peer.getModel.getSize > 1) 
        fnLbl.text = "Please select the column which contains "+
          "the file names for the new documents"
    }
  }
}

Answer 1

The code you posted is rather huge for all-in-one review, so I suggest to make it in more than one iteration.

For this first approach, let's cut more general things from details.

InteractionMediator

Type. It is declared as case class. Is it intended to be used in matchers or to be tested for equality among different instances? If not, that should not be a case class.

Functions. There are really a lot of public defs inside. Are they all called from outside or some of them remain callable from within the class? I think that it should be useful to reduce the visibility of some of them.

Moreover, these functions do many different things and this makes me doubt that InteractionMediator should wrap all this stuff. There are functions to validate details or templates, load them or generate other entities. It looks that a split is necessary: create an entity that will provide validation, another for loading, and yet another for other useful actions. This will be much easier to read, analyze and test.

Wizard

This class looks like a big mess of vars, vals, defs and directly executed calls. Really difficult to see what is happening there. Reordering, restructuring and splitting are the first things to do about.

Some ideas:

1. Do not hesitate to extract the initialization of FileChoosers, ComboBoxetc in dedicated methods, it will allow to instantiate the respective fields in one line without messy lines after.

2. Group the instructions executed directly through the constructor into a dedicated function (that might also call other functions of grouped calls) and call this function once, as close as possible to the header of the class.

Once this initial refactoring is done, it would be much easier to understand how the entire thing works and to review the details.