4
\$\begingroup\$

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 ...

\$\endgroup\$
7
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Very good explanation. I wrote a lot of imperative code before in different programming languages, so I avoid for in case to iterate over list, I try to use it only for list comprehension. But the examples often impractical, thanks for this one :) Cool story about defensive code. I try to ask myself "Am I write defensive code ?" :) \$\endgroup\$ – Valery Feb 24 '15 at 10:51
  • \$\begingroup\$ Glad I could help. Unrelated to your question, @Valery... it would be good practice to add an explicit return type to the definition of your read function. Because you don't, read's return type will match whatever is the last expression in the code. If you accidentally change that, you will not notice. You are particularly in danger of this because your final expression is an if block. It is possible to return one type for true and another for false, in which case Scala will look for a common supertype. \$\endgroup\$ – itsbruce Feb 24 '15 at 14:25
  • \$\begingroup\$ For example, this code def foo(i: Int) = i match { case 0 => "zero"; case _ => i; } creates a function with return type Any. It is good practice to specify the return type of most functions, particularly any that are exported. \$\endgroup\$ – itsbruce Feb 24 '15 at 14:27
-1
\$\begingroup\$
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.

\$\endgroup\$
  • 5
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – Hosch250 Nov 20 '15 at 19:46
  • \$\begingroup\$ @Hosch250 It is not just an alternative solution but a tested working one to make sure there is a working implementation on the web that way other people like me who are looking for a solution to excel reading in scala can use this and modify this to their needs. I appreciate you looking out for the OP but this was posted a long time ago and I wanted make a reference for aspiring students like myself. So down voting a working solution is in my opinion very counter productive to the thread. If I missed anything in my explanation then feel free to elaborate in the comments. Hope this can help \$\endgroup\$ – Stephen09 Nov 23 '15 at 16:42
  • \$\begingroup\$ Answers should review OP's code. Posts that don't relate to the OP are not answers. Comments are not the place for this discussion, feel free to post on Code Review Meta if you have any questions or concerns. \$\endgroup\$ – Mathieu Guindon Nov 24 '15 at 15:02

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.