Extracting tuples from Excel sheet

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()

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  (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 ;)

StackExchange Code Review Q#81908, answer score: 7