Refactor code

ONSdigital · Jul 12, 2021 · d415aff · d415aff
1 parent c79abdd
commit d415aff
Show file tree

Hide file tree

Showing 6 changed files with 143 additions and 108 deletions.
diff --git a/src/main/scala/Main.scala b/src/main/scala/Main.scala
@@ -22,13 +22,13 @@ object Main extends App {
       if (errorsAndWarnings.warnings.nonEmpty) {
         println(Console.YELLOW + "Warnings")
         errorsAndWarnings.warnings.foreach(x =>
-          logger.warn(processWarningsAndErrors(x))
+          logger.warn(getDescriptionForError(x))
         )
       }
       if (errorsAndWarnings.errors.nonEmpty) {
         println(Console.RED + "Error")
         errorsAndWarnings.errors.foreach(x =>
-          logger.warn(processWarningsAndErrors(x))
+          logger.warn(getDescriptionForError(x))
         )
         print(Console.RESET + "")
         sys.exit(1)
@@ -49,7 +49,7 @@ object Main extends App {
     }
   }
 
-  private def processWarningsAndErrors(
+  private def getDescriptionForError(
       errorMessage: ErrorWithCsvContext
   ): String = {
     s"Type: ${errorMessage.`type`}, Category: ${errorMessage.category}, " +

diff --git a/src/main/scala/Table.scala b/src/main/scala/Table.scala
@@ -144,6 +144,21 @@ object Table {
       .orElse(inheritedPropertiesCopy.get("tableSchema"))
   }
 
+  private def getDialect(
+      tableProperties: Map[String, JsonNode]
+  ): Option[Dialect] = {
+    tableProperties
+      .get("dialect")
+      .flatMap {
+        case d: ObjectNode => Some(Dialect.fromJson(d))
+        case d if d.isNull => None
+        case d =>
+          throw MetadataError(
+            s"Unexpected JsonNode type ${d.getClass.getName}"
+          )
+      }
+  }
+
   private def createTableForExistingSchema(
       tableSchema: JsonNode,
       inheritedProperties: Map[String, JsonNode],
@@ -192,16 +207,7 @@ object Table {
           url = url,
           id = getId(tableProperties),
           columns = columns,
-          dialect = tableProperties
-            .get("dialect")
-            .flatMap {
-              case d: ObjectNode => Some(Dialect.fromJson(d))
-              case d if d.isNull => None
-              case d =>
-                throw MetadataError(
-                  s"Unexpected JsonNode type ${d.getClass.getName}"
-                )
-            },
+          dialect = getDialect(tableProperties),
           foreignKeys = foreignKeyMappings, // a new type here?
           notes = getNotes(tableProperties),
           primaryKey = primaryKeyToReturn,
@@ -575,24 +581,43 @@ case class Table private (
         ValidateRowOutput(
           WarningsAndErrors(Array(), errors),
           primaryKeyValues,
-          foreignKeyReferenceValues
-            .groupBy {
-              case (k, _) => k
-            }
-            .map {
-              case (k, values) =>
-                (k, KeyWithContext(row.getRecordNumber, values.map(v => v._2)))
-            },
-          foreignKeyValues
-            .groupBy { case (k, _) => k }
-            .map {
-              case (k, values) =>
-                (k, KeyWithContext(row.getRecordNumber, values.map(v => v._2)))
-            }
+          getParentTableForeignKeys(foreignKeyReferenceValues, row),
+          getChildForeignKeys(foreignKeyValues, row)
         )
       )
     } else None
   }
+
+  private def getChildForeignKeys(
+      foreignKeyValues: List[(ChildTableForeignKey, List[Any])],
+      row: CSVRecord
+  ): Predef.Map[ChildTableForeignKey, KeyWithContext] = {
+    foreignKeyValues
+      .groupBy {
+        case (k, _) => k
+      }
+      .map {
+        case (k, values) =>
+          (k, KeyWithContext(row.getRecordNumber, values.map(v => v._2)))
+      }
+  }
+
+  private def getParentTableForeignKeys(
+      foreignKeyReferenceValues: List[
+        (ParentTableForeignKeyReference, List[Any])
+      ],
+      row: CSVRecord
+  ): Predef.Map[ParentTableForeignKeyReference, KeyWithContext] = {
+    foreignKeyReferenceValues
+      .groupBy {
+        case (k, _) => k
+      }
+      .map {
+        case (k, values) =>
+          (k, KeyWithContext(row.getRecordNumber, values.map(v => v._2)))
+      }
+  }
+
   def validateHeader(
       header: CSVRecord
   ): WarningsAndErrors = {

diff --git a/src/main/scala/Validator.scala b/src/main/scala/Validator.scala
@@ -55,11 +55,11 @@ class Validator(var tableCsvFile: URI, sourceUri: String = "") {
       val table = schema.tables(tableUrl)
       val tableUri = new URI(tableUrl)
       val dialect = table.dialect.getOrElse(Dialect())
-      val format = setCsvFormat(dialect)
+      val format = getCsvFormat(dialect)
       readAndValidateCsv(schema, tableUri, format, dialect) match {
-        case Some(value) => {
-          allForeignKeyValues(table) = value._1
-          allForeignKeyReferenceValues(table) = value._2
+        case Some((foreignKeys, foreignKeyReferences)) => {
+          allForeignKeyValues(table) = foreignKeys
+          allForeignKeyReferenceValues(table) = foreignKeyReferences
         }
         case None => {}
       }
@@ -70,7 +70,7 @@ class Validator(var tableCsvFile: URI, sourceUri: String = "") {
     )
   }
 
-  def setCsvFormat(dialect: Dialect): CSVFormat = {
+  def getCsvFormat(dialect: Dialect): CSVFormat = {
     CSVFormat.DEFAULT
       .withDelimiter(dialect.delimiter)
       .withQuote(dialect.quoteChar)
@@ -82,20 +82,35 @@ class Validator(var tableCsvFile: URI, sourceUri: String = "") {
   def getParser(
       tableUri: URI,
       dialect: Dialect,
-      format: CSVFormat,
-      tableCsvFile: File
-  ): CSVParser = {
+      format: CSVFormat
+  ): Option[CSVParser] = {
     if (tableUri.getScheme == "file") {
-      CSVParser.parse(
-        tableCsvFile,
-        mapAvailableCharsets(dialect.encoding),
-        format
+      val tableCsvFile = new File(tableUri)
+      if (!tableCsvFile.exists) {
+        errors :+= ErrorWithCsvContext(
+          "file_not_found",
+          "",
+          "",
+          "",
+          s"File named ${tableUri.toString} cannot be located",
+          ""
+        )
+        return None
+      }
+      Some(
+        CSVParser.parse(
+          tableCsvFile,
+          mapAvailableCharsets(dialect.encoding),
+          format
+        )
       )
     } else {
-      CSVParser.parse(
-        tableUri.toURL,
-        mapAvailableCharsets(dialect.encoding),
-        format
+      Some(
+        CSVParser.parse(
+          tableUri.toURL,
+          mapAvailableCharsets(dialect.encoding),
+          format
+        )
       )
     }
   }
@@ -111,60 +126,56 @@ class Validator(var tableCsvFile: URI, sourceUri: String = "") {
         Map[ParentTableForeignKeyReference, Set[KeyWithContext]]
     )
   ] = {
-    val tableCsvFile = new File(tableUri)
-    if (!tableCsvFile.exists) {
-      errors :+= ErrorWithCsvContext(
-        "file_not_found",
-        "",
-        "",
-        "",
-        s"File named ${tableUri.toString} cannot be located",
-        ""
-      )
-      return None
-    }
-    val parser = getParser(tableUri, dialect, format, tableCsvFile)
-    val parserAfterSkippedRows = parser.asScala.drop(dialect.skipRows)
-    val table = schema.tables(tableUri.toString)
-    val childTableForeignKeys =
-      Map[ChildTableForeignKey, Set[KeyWithContext]]()
-    val parentTableForeignKeyReferences =
-      Map[ParentTableForeignKeyReference, Set[KeyWithContext]]()
-    val allPrimaryKeyValues: Set[List[Any]] =
-      Set() // List of type Any with same values are not added again in a Set, whereas Array of Type any behaves differently.
-    for (row <- parserAfterSkippedRows) {
-      System.out.print(".")
-      if (row.getRecordNumber == 1 && dialect.header) {
-        validateHeader(schema, row, tableUri)
-      } else {
-        if (row.size == 0) {
-          warnings :+= ErrorWithCsvContext(
-            "Blank rows",
-            "structure",
-            row.getRecordNumber.toString,
-            "",
-            "",
-            ""
-          )
-        }
+    val maybeParser = getParser(tableUri, dialect, format)
+    maybeParser match {
+      case Some(parser) => {
+        val parserAfterSkippedRows = parser.asScala.drop(dialect.skipRows)
+        val table = schema.tables(tableUri.toString)
+        val childTableForeignKeys =
+          Map[ChildTableForeignKey, Set[KeyWithContext]]()
+        val parentTableForeignKeyReferences =
+          Map[ParentTableForeignKeyReference, Set[KeyWithContext]]()
+        val allPrimaryKeyValues: Set[List[Any]] =
+          Set() // List of type Any with same values are not added again in a Set, whereas Array of Type any behaves differently.
+        for (row <- parserAfterSkippedRows) {
+          if (row.getRecordNumber == 1 && dialect.header) {
+            validateHeader(schema, row, tableUri)
+          } else {
+            if (row.size == 0) {
+              warnings :+= ErrorWithCsvContext(
+                "Blank rows",
+                "structure",
+                row.getRecordNumber.toString,
+                "",
+                "",
+                ""
+              )
+            }
 
-        val result = table.validateRow(row)
-        result match {
-          case Some(validateRowOutput) => {
-            errors ++= validateRowOutput.warningsAndErrors.errors
-            warnings ++= validateRowOutput.warningsAndErrors.warnings
-            setChildTableForeignKeys(validateRowOutput, childTableForeignKeys)
-            setParentTableForeignKeyReferences(
-              validateRowOutput,
-              parentTableForeignKeyReferences
-            )
-            validatePrimaryKeys(allPrimaryKeyValues, row, validateRowOutput)
+            val result = table.validateRow(row)
+            result match {
+              case Some(validateRowOutput) => {
+                errors ++= validateRowOutput.warningsAndErrors.errors
+                warnings ++= validateRowOutput.warningsAndErrors.warnings
+                setChildTableForeignKeys(
+                  validateRowOutput,
+                  childTableForeignKeys
+                )
+                setParentTableForeignKeyReferences(
+                  validateRowOutput,
+                  parentTableForeignKeyReferences
+                )
+                validatePrimaryKeys(allPrimaryKeyValues, row, validateRowOutput)
+              }
+              case None => {}
+            }
           }
-          case None => {}
         }
+        Some(childTableForeignKeys, parentTableForeignKeyReferences)
       }
+      case None => None
     }
-    Some(childTableForeignKeys, parentTableForeignKeyReferences)
+
   }
 
   private def validatePrimaryKeys(
@@ -254,27 +265,23 @@ class Validator(var tableCsvFile: URI, sourceUri: String = "") {
       ) {
         val childTableForeignKeys
             : Map[ChildTableForeignKey, Set[KeyWithContext]] =
-          childTableForeignKeysByTable.get(
-            parentTableForeignKeyReference.childTable
-          ) match {
-            case Some(x) => x
-            case None =>
+          childTableForeignKeysByTable
+            .get(parentTableForeignKeyReference.childTable)
+            .getOrElse(
               throw new Exception(
                 s"Could not find corresponding child table for parent table ${parentTable.url}"
               )
-          }
+            )
 
         val childTableKeyValues: Set[KeyWithContext] =
-          childTableForeignKeys.get(
-            parentTableForeignKeyReference.foreignKey
-          ) match {
-            case Some(x) => x
-            case None =>
+          childTableForeignKeys
+            .get(parentTableForeignKeyReference.foreignKey)
+            .getOrElse(
               throw new Exception(
-                s"Could not find foreign key against child table. " +
-                  parentTableForeignKeyReference.foreignKey.jsonObject.toPrettyString
+                s"Could not find foreign key against child table." + parentTableForeignKeyReference.foreignKey.jsonObject.toPrettyString
               )
-          }
+            )
+
         val keyValuesNotDefinedInParent =
           childTableKeyValues -- allPossibleParentTableValues
         if (keyValuesNotDefinedInParent.nonEmpty) {

diff --git a/src/main/scala/models/KeyWithContext.scala b/src/main/scala/models/KeyWithContext.scala
@@ -4,6 +4,9 @@ case class KeyWithContext(
     keyValues: List[Any],
     var isDuplicate: Boolean = false
 ) {
+  // KeyWithContext object holds the rowNumber information for setting better errors.
+  // Thus `rowNumber` attribute doesn't need to be considered when checking the equality of 2 KeyWithContext objects
+  // or for the hashCode of the KeyWithContext object.
 
   override def equals(obj: Any): Boolean =
     obj != null &&

diff --git a/src/test/scala/ValidatorTest.scala b/src/test/scala/ValidatorTest.scala
@@ -201,7 +201,7 @@ class ValidatorTest extends FunSuite {
   }
 
   test(
-    "it not add second key-value with the exact same values and different row number"
+    "it should not add second key-value with the exact same values and different row number"
   ) {
 
     val set = mutable.Set[KeyWithContext]()

diff --git a/src/test/scala/steps/RunCucumberTest.scala b/src/test/scala/steps/RunCucumberTest.scala
@@ -3,4 +3,4 @@
 //
 //@RunWith(classOf[Cucumber])
 //@CucumberOptions(features = Array("src/test/resources/features"))
-//class RunCucumberTest
+//class RunCucumberTest
-Original file line number
+Diff line change
@@ Expand Up / @@ -201,7 +201,7 @@ class ValidatorTest extends FunSuite { @@
       }
       test(
-        "it not add second key-value with the exact same values and different row number"
+        "it should not add second key-value with the exact same values and different row number"
       ) {
         val set = mutable.Set[KeyWithContext]()
@@ Expand Down @@