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.

• Claudius: Great, this is a good start already. I may need a couple of days to work on all the changes, as this is a non-work related project so I'm using my free time for it. When I have made them all, I'll edit the question with the new source code. Thanks for taking the time to respond!
• Antot: @claudiusbr: you are welcome. But when the changes in the code are ready, it would be better to post them as another "follow up" question, without editing this one. It keeps questions and answers more consistent.
• Claudius: Ah! Great. I was actually also going to ask about how to move forward question-wise, but wanted to re-read the guidelines first before I did. I'm still gonna re-read them, in any case, but thanks for the pointer. I'll accept this answer then, as it has already given me a lot to work on, and will post the second iteration as a follow up. Thanks again.