Extracting tuples from Excel sheet

Question

My code have too much nested option processing which makes it ugly. But it has an advantage it has no side effect and typesafe.

package repos

import java.io.{InputStream, File}
import org.apache.poi.hssf.usermodel.HSSFWorkbook
import org.apache.poi.ss.usermodel.{Cell, Row, Workbook, Sheet}
import org.apache.poi.xssf.usermodel._
import scala.collection.JavaConversions._

object ExcelRepo{

  def read(binary: InputStream, filename: String) = {
    val excelExtension = "([^\\s]+(\\.(?i)(xls|xlsx))$)"
    val excelData = if(filename.matches(excelExtension)) {

      val xlsExtension = "([^\\s]+(\\.(?i)(xls))$)"
      val xlsxExtension = "([^\\s]+(\\.(?i)(xlsx))$)"

      val workbook:Option[Workbook] = if (filename.matches(xlsExtension)) {
        println("XLS")
        Some(new HSSFWorkbook(binary))
      } else {
        println("XLSX")
        Some(new XSSFWorkbook(binary))
      }

      val data = workbook match {
        case Some(wb: Workbook) =>
          val sheet = Option(wb.getSheetAt(0))
          sheet match {
            case Some(s: Sheet) =>
              val titleRow = Option(s.getRow(0))
              titleRow match {
                case Some(tr:Row) =>
                  val titleCell = Option(tr.getCell(0))
                  val priceCell = Option(tr.getCell(1))
                  (titleCell, priceCell) match {
                    case (Some(tc: Cell), Some(pc:Cell)) =>
                      if (pc.getStringCellValue == "price" && tc.getStringCellValue == "title") {
                        val list:List[(String, Double)] = sheet.get.tail.map{
                         r => (r.getCell(0).getStringCellValue, r.getCell(1).getNumericCellValue)}.toList
                        list
                      } else {
                        List()
                      }
                    case (_,_) => List()
                  }
                case None => List()
              }
            case None => List()
          }
        case None => List()
      }
      data
    } else {
      List()
    }
    excelData
  }

}

This function reads Excel file assuming that it is not null, and returning the List of tuples with (price, title), or empty list if workbook or row or cell is empty.

Is it actually ugly? Should I refactor it? If I should what is the clear and concise way to deal with match Option → getting new Option → match it again → getting new Option ...

Answer 1

You are doing the same thing in case of failure at any stage - returning an empty list. You should use a for comprehension to chain these stages; if any one stage returns None, then the other stages will not be followed. If you do something like this:

  for {
    wb <- workbook;
    sheet <- Option(wb.getSheetAt(0));
    titleRow <- Option(sheet.getRow(0));
    titleCell <- Option(titleRow.getCell(0)) if titleCell.getStringCellValue == "title";
    priceCell <- Option(titleRow.getCell(1)) if priceCell.getStringCellValue == "price";
  } yield { sheet.get.tail.map{r => (r.getCell(0).getStringCellValue, r.getCell(1).getNumericCellValue)}.toList
  }

This will return Option[List[(String, Double)]], containing either a real list or None. If any of those stages returns None, the following stages will not be executed and you just get your None back early. You can pattern match against this output to return the contents of Some(list) or List() if the output is None.

Note: I took the annotated val list out of there, so you would either need to have specified the return type for a val to which you assign the output of the for comprehension or put it back into the yield block.

Note how I turned your if...else expression into guards. Can you see just how much simpler the code is and how each conditional is paired with the appropriate object? Particularly since the else was just returning another empty list. This way, it fails and returns None.

"for comprehensions" can be overused in Scala, but this really is where they shine. You correctly chose Option, but possibly the best thing about Option for any Java coder is the way it can remove boilerplate and defensive coding. Your code still has the defensive coding, while the for comprehension just throws all that away. Learn to be free ;)

Answer 2

package cs220.ds

import java.io.{InputStream, File, FileInputStream}
import org.apache.poi.hssf.usermodel.HSSFWorkbook
import org.apache.poi.ss.usermodel.{Cell, Row, Workbook, Sheet}
import org.apache.poi.xssf.usermodel._
import scala.collection.JavaConversions._

object ExcelRepo{

  def read(binary: InputStream, filename: String)= {
    val excelExtension = "([^\\s]+(\\.(?i)(xls|xlsx))$)"
    val excelData = if(filename.matches(excelExtension)) {

      val xlsExtension = "([^\\s]+(\\.(?i)(xls))$)"
      val xlsxExtension = "([^\\s]+(\\.(?i)(xlsx))$)"

      val workbook:Option[Workbook] = if (filename.matches(xlsExtension)) {
        println("XLS")
        Some(new HSSFWorkbook(binary))
      } else {
        println("XLSX")
        Some(new XSSFWorkbook(binary))
      }

      val data  = {
        for {
          wb <- workbook
          sheet <- Option( wb.getSheetAt(0) )
        } yield { sheet.tail.map( r => (r.getCell(0), r.getCell(1),r.getCell(2)) ).toList}
      }

        data
      }

print("output:" + excelData)
     excelData
  }

}

This is a working implementation that I realized you can not use the get function on sheet due to tail only applies to Option, and .get will actually pull what's in the Option. which will throw an error, What this code is the combination of the OP's code with the provided solution that has been fixed for minor errors that would make it not work correctly. I removed many unnecessary checks for my use to just pull all but the header row in excel. I used the println in the bottom of the function to make sure I was getting the data I wanted. I also added data at the end of the read FN to make sure that the correct variable is returned from the excelData value. This is all I have done to make this project work.