Code review

build.sbt

@@ -5,7 +5,8 @@ version := "0.4.2"
 scalaVersion := "2.11.7"
 
 organization := "com.onzo"
-
+//compiler options are not used. Should revise the options and decide which to keep
+//scalacOptions ++= compilerOptions //uncomment after revision
 lazy val compilerOptions = Seq(
   "-deprecation",
   "-encoding", "UTF-8",
@@ -18,26 +19,30 @@ lazy val compilerOptions = Seq(
   "-Ywarn-numeric-widen",
   "-Xfuture"
 )
-
-resolvers += "Sonatype Snapshots" at "https://oss.sonatype.org/content/repositories/snapshots"
+//canonical way to add sonatype snapshots: http://www.scala-sbt.org/0.13/docs/Resolvers.html
+resolvers += Resolver.sonatypeRepo("snapshots")
 
 resolvers += Resolver.bintrayRepo("dwhjames", "maven")
-libraryDependencies += "com.github.dwhjames" %% "aws-wrap" % "0.8.0"
-libraryDependencies += "com.amazonaws" % "aws-java-sdk-dynamodb" % "1.10.34" % "provided"
-
-lazy val catsVersion = "0.4.1"
-
-libraryDependencies ++= Seq(
-  "org.typelevel" %% "cats" % catsVersion,
-  "com.chuusai" %% "shapeless" % "2.3.0",
-  "org.scalacheck" %% "scalacheck" % "1.12.5" % "test",
-  "org.scalatest" %% "scalatest" % "3.0.0-M9" % "test",
-  "joda-time" % "joda-time" % "2.9" % "test",
-  "org.joda"  % "joda-convert" % "1.8" % "test"
+
+val dynamoDbDeps = Seq(
+  //https://github.com/dwhjames/aws-wrap
+  "com.github.dwhjames" %% "aws-wrap" % "0.8.0",
+  "com.amazonaws" % "aws-java-sdk-dynamodb" % "1.10.34" % "provided") //Latest version: 1.11.122
+val scalaUtilities = Seq(
+  //No need to assign variable for version of single dependency
+  "org.typelevel" %% "cats" % "0.4.1", //Latest version: "0.9.0"
+  "com.chuusai" %% "shapeless" % "2.3.0" //Latest version is "2.3.2"
 )
-// Bintray:
+val testingDeps = Seq(
+  "org.scalacheck" %% "scalacheck" % "1.12.5", //Latest version: "1.13.4"
+  "org.scalatest" %% "scalatest" % "3.0.0-M9", //Latest version: "3.0.1"
+  "joda-time" % "joda-time" % "2.9", //Latest version: "2.9.9"
+  "org.joda" % "joda-convert" % "1.8").map(_ % "test")
+
+libraryDependencies ++= dynamoDbDeps ++ scalaUtilities ++ testingDeps
 
-licenses +=("Apache-2.0", url("http://apache.org/licenses/LICENSE-2.0"))
+// Bintray:
+licenses += ("Apache-2.0", url("http://apache.org/licenses/LICENSE-2.0"))
 
 bintrayPackageLabels in bintray := Seq("scala", "dynamo", "dynamodb", "amazon", "utility")
 

src/main/scala/com/onzo/dynamodb/Decoder.scala

@@ -9,28 +9,25 @@ import com.amazonaws.services.dynamodbv2.model.AttributeValue
 import scala.collection.generic.CanBuildFrom
 import scala.util.Try
 
-trait Decoder[A] {
-  self =>
+//Decoder is a actually just a function AttributeValue => A
+trait Decoder[A] extends (AttributeValue => A) {
   def apply(c: AttributeValue): A
 
-  def apply(name: String, items: Map[String, AttributeValue]): A = {
-    val vOpt = items.get(name)
-    vOpt.fold(
-      throw new Exception(s"Attribute '$name' not found in '$items'")
-    )(
-      v => apply(v)
-    )
-  }
+  //A safer method that does not throw.
+  def get(c: AttributeValue): Option[A] = Try(apply(c)).toOption
 
-  def map[B](f: A => B): Decoder[B] = new Decoder[B] {
-    def apply(c: AttributeValue): B = f(self(c))
+  def apply(name: String, items: Map[String, AttributeValue]): A = items.get(name) match {
+    //I find pattern matching on Option to convey intent better than fold
+    case Some(v) => apply(v)
+    case None => throw new Exception(s"Attribute '$name' not found in '$items'")
   }
 
-  def flatMap[B](f: A => Decoder[B]): Decoder[B] = new Decoder[B] {
-    def apply(c: AttributeValue): B = {
-      f(self(c))(c)
-    }
-  }
+  def map[B](f: A => B): Decoder[B] = Decoder.instance(this andThen f)
+
+  /* This flatMap consumes once the AttributeValue to create a new decoder, then decodes the same AttributeValue with the new decoder.
+     TODO: Check if this is indeed useful for decoding AttributeValue
+    */
+  def flatMap[B](f: A => Decoder[B]): Decoder[B] = Decoder.instance(c => this.andThen(f)(c)(c))
 }
 
 object Decoder {
@@ -43,60 +40,57 @@ object Decoder {
   implicit val decodeAttributeValue: Decoder[AttributeValue] = instance(identity)
   implicit val decodeString: Decoder[String] = instance(_.getS)
   implicit val decodeBoolean: Decoder[Boolean] = instance(_.getBOOL)
-  implicit val encodeFloat: Decoder[Float] = instance(_.getN.toFloat)
-  implicit val encodeDouble: Decoder[Double] = instance(_.getN.toDouble)
-  implicit val encodeByte: Decoder[Byte] = instance(_.getN.toByte)
-  implicit val encodeShort: Decoder[Short] = instance(_.getN.toShort)
-  implicit val encodeInt: Decoder[Int] = instance(_.getN.toInt)
-  implicit val encodeLong: Decoder[Long] = instance(_.getN.toLong)
-  implicit val encodeBigInt: Decoder[BigInt] = instance(a => BigDecimal(a.getN, MathContext.UNLIMITED).toBigInt())
-  implicit val encodeBigDecimal: Decoder[BigDecimal] = instance(a => BigDecimal(a.getN, MathContext.UNLIMITED))
-  implicit val encodeUUID: Decoder[UUID] = instance(a => UUID.fromString(a.getS))
-
+  implicit val decodeFloat: Decoder[Float] = instance(_.getN.toFloat)
+  implicit val decodeDouble: Decoder[Double] = instance(_.getN.toDouble)
+  implicit val decodeByte: Decoder[Byte] = instance(_.getN.toByte)
+  implicit val decodeShort: Decoder[Short] = instance(_.getN.toShort)
+  implicit val decodeInt: Decoder[Int] = instance(_.getN.toInt)
+  implicit val decodeLong: Decoder[Long] = instance(_.getN.toLong)
+  implicit val decodeBigInt: Decoder[BigInt] = instance(a => BigDecimal(a.getN, MathContext.UNLIMITED).toBigInt())
+  implicit val decodeBigDecimal: Decoder[BigDecimal] = instance(a => BigDecimal(a.getN, MathContext.UNLIMITED))
+  implicit val decodeUUID: Decoder[UUID] = instance(a => UUID.fromString(a.getS))
+
+  //I would prefer to get a concrete List / Vector back and convert to desired collection explicitly if needed.
   implicit def decodeCanBuildFrom[A, C[_]](implicit
                                            d: Decoder[A],
                                            cbf: CanBuildFrom[Nothing, A, C[A]]
                                           ): Decoder[C[A]] = instance { c =>
-    import scala.collection.JavaConversions._
-
-    val list = c.getL
+    import scala.collection.JavaConverters._
     val builder = cbf()
-    for {
-      e <- list
-    } yield {
-      builder += d(e)
-    }
+    val list = c.getL.asScala
+    list.foreach(e => builder += d(e))
     builder.result()
   }
-
+  //This adds different semantics to 2nd apply, since it makes it tolerant to both missing name and wrong conversion.
+  //I would like to know if the name was not found or the conversion failed and treat them differently
   implicit def decodeOption[A](implicit d: Decoder[A]): Decoder[Option[A]] = new Decoder[Option[A]] {
     override def apply(c: AttributeValue): Option[A] = Try(d(c)).toOption
     override def apply(name: String, items: Map[String, AttributeValue]): Option[A] = {
       items.get(name).flatMap(apply)
     }
   }
-
-  /**
-    * @group Decoding
-    */
+  //CanBuildFrom for creating a sub-class of Map sounds very far-fetched. I would stick to direct conversion to Map.
   implicit def decodeMap[M[K, +V] <: Map[K, V], V](implicit
                                                    d: Decoder[V],
                                                    cbf: CanBuildFrom[Nothing, (String, V), M[String, V]]
                                                   ): Decoder[M[String, V]] = instance { c =>
-    import scala.collection.JavaConversions._
-    val map = c.getM
+    import scala.collection.JavaConverters._
     val builder = cbf()
-    for {
-      (k, v) <- map
-    } yield {
-      builder += k -> d(v)
-    }
+    val map = c.getM.asScala
+    map.foreach { case (k, v) => builder += k -> d(v) }
     builder.result()
   }
-
+  /*
+    Decoder is a Functor, but I am not sure if it should be a monad too. (Check comment on flatMap above)
+    Consider using just this instead.
+    implicit val functorDecoder: Functor[Decoder] = new Functor[Decoder] {
+      override def map[A, B](fa: Decoder[A])(f: (A) => B): Decoder[B] = fa.map(f)
+    }
+  */
   implicit val monadDecode: Monad[Decoder] = new Monad[Decoder] {
     def pure[A](a: A): Decoder[A] = instance(_ => a)
 
     def flatMap[A, B](fa: Decoder[A])(f: A => Decoder[B]): Decoder[B] = fa.flatMap(f)
   }
+
 }

src/main/scala/com/onzo/dynamodb/Encoder.scala

@@ -1,17 +1,19 @@
 package com.onzo.dynamodb
 
 import java.util.UUID
+
 import cats.functor.Contravariant
 import com.amazonaws.services.dynamodbv2.model.AttributeValue
+
 import scala.collection.generic.IsTraversableOnce
 
-trait Encoder[A] {
-  //self =>
+trait Encoder[A] extends (A => AttributeValue){
+  //TODO: Understand how this is used vs the 2nd apply
   def apply(a: A): AttributeValue
 
-  def apply(name: String, a: A): Map[String, AttributeValue] = {
-    Map(name -> apply(a))
-  }
+  //This is the main apply method that is used in the tests
+  //TODO:Why not return (String, AttributeValue) or Option[(String, AttributeValue)]
+  def apply(name: String, a: A): Map[String, AttributeValue] = Map(name -> apply(a))
 
   def contramap[B](f: B => A): Encoder[B] = Encoder.instance(b => apply(f(b)))
 }
@@ -29,11 +31,7 @@ object Encoder {
                                               ): Encoder[C[A0]] =
     instance { list =>
       val items = new java.util.ArrayList[AttributeValue]()
-
-      is.conversion(list).foreach { a =>
-        items add e(a)
-      }
-
+      is.conversion(list).foreach(a => items add e(a))
       new AttributeValue().withL(items)
     }
 
@@ -50,34 +48,27 @@ object Encoder {
   implicit val encodeBigDecimal: Encoder[BigDecimal] = instance(a => new AttributeValue().withN(a.toString))
   implicit val encodeUUID: Encoder[UUID] = instance(uuid => new AttributeValue().withS(uuid.toString))
 
+  //DynamoDB can encode null, but previous implementation was throwing for None. Not sure yet if this
   implicit def encodeOption[A](implicit e: Encoder[A]): Encoder[Option[A]] = new Encoder[Option[A]] {
-    //self =>
-    override def apply(a: Option[A]): AttributeValue = e(a.get)
-
-    override def apply(name: String, a: Option[A]): Map[String, AttributeValue] = {
-      if(a.isDefined)
-        Map(name -> apply(a))
-      else
-        Map.empty[String,AttributeValue]
-    }
+    //TODO: Check the way the DynamoDB api treats nulls.
+    override def apply(a: Option[A]): AttributeValue = a.map(e).getOrElse(new AttributeValue().withNULL(true))
+    //TODO: Check what it means to give back empty Map for None.
+    override def apply(name: String, a: Option[A]): Map[String, AttributeValue] =
+      a.map(v => Map(name -> e(v))).getOrElse(Map.empty)
   }
 
-
   implicit def encodeMapLike[M[K, +V] <: Map[K, V], V](implicit
                                                        e: Encoder[V]
                                                       ): Encoder[M[String, V]] = Encoder.instance { m =>
-    val map = m.map {
-      case (k, v) => (k, e(v))
-    }
-    import scala.collection.JavaConversions._
-
-    new AttributeValue().withM(map)
+    //Intellij is strugling with overloaded apply methods so I added explicit type signature on map
+    val map: Map[String, AttributeValue] = m.mapValues(e)
+    import scala.collection.JavaConverters._
+    new AttributeValue().withM(map.asJava)
   }
-
-  def encodeEither[A, B](leftKey: String, rightKey: String)(implicit
-                                                            ea: Encoder[A],
-                                                            eb: Encoder[B]
-  ): Encoder[Either[A, B]] = instance { a =>
+  // There is no Decoder[Either]. To decode to Either we need to know the key.
+  // I would instead avoid Either and use sealed trait with explicit subtype name, or use the class name as key.
+  def encodeEither[A, B](leftKey: String, rightKey: String)(implicit ea: Encoder[A], eb: Encoder[B]): Encoder[Either[A, B]] =
+  instance { a =>
     val map = new java.util.HashMap[String, AttributeValue]()
     a.fold(
       a => map.put(leftKey, ea(a)),

src/main/scala/com/onzo/dynamodb/Key.scala

@@ -29,9 +29,11 @@ case class RangeKey[A](name: String)(implicit val encoder: Encoder[A], val decod
 case class Key[A](name: String)(implicit val encoder: Encoder[A], val decoder: Decoder[A])
   extends NamedKeyLike[A]
 
+
 /**
-  * This key return the rest of the dynamodb columns not mapped by other the keys
-  * This key write it's Map[String, A] as column in dynamodb
+  * This key returns the rest of the dynamodb columns not mapped by other the keys
+  * This key writes its Map[String, A] as column in dynamodb
+  *
   * @param encoder
   * @param decoder
   * @tparam A
@@ -40,6 +42,7 @@ case class MapKey[A](implicit val encoder: Encoder[A], val decoder: Decoder[A])
   extends KeyLike[Map[String, A]] {
   def encode(t: Map[String, A]): Map[String, AttributeValue] = {
     val mapB = Map.newBuilder[String, AttributeValue]
+    t.map{case (key, v) => mapB ++= encoder(key, v)}
     t.foreach {
       case (key, v) => mapB ++= encoder(key, v)
     }

src/main/scala/com/onzo/dynamodb/Table.scala

@@ -3,7 +3,6 @@ package com.onzo.dynamodb
 import com.github.dwhjames.awswrap.dynamodb._
 import scala.collection.mutable
 
-
 abstract class Table[T](override val tableName: String) extends DynamoDBSerializer[T] {
 
   val * : TableMapper[T]

src/main/scala/com/onzo/dynamodb/TableMapper.scala

@@ -1,9 +1,10 @@
 package com.onzo.dynamodb
 
 import com.amazonaws.services.dynamodbv2.model.AttributeValue
-
+//TODO: Think if we can create a com.amazonaws.services.dynamodbv2.model.CreateTableRequest automatically from TableMapper + a few more metadata
 trait TableMapper[A] {
   self =>
+  // Encoder is a type class. Why carry this information instead of resolving it. Reduces ad-hoc polymorphism opportunities.
 
   def primaryKey: Option[(String, Encoder[A])]
 

src/main/scala/com/onzo/dynamodb/package.scala

@@ -5,9 +5,16 @@ import shapeless.LUBConstraint._
 import shapeless._
 import shapeless.ops.hlist._
 
-
 package object dynamodb {
 
+  /*
+  This is a very complicated type-level logic that most Scala programmers would struggle with.
+  When implicit resolution fails, users will have to go through this code to understand what is really missing or how to fix it at call site.
+  I would add some more test methods around this and comments, at least explaining each type argument and implicit arguments.
+
+  From a type safety percpective, keys are not marked with the type-level record name
+  so I expect the `as` method to only match against the sequence of types, not checking field names. I would investigate using LabelledGeneric instead.
+  */
   implicit class KeysHList[
   A <: HList : <<:[KeyLike[_]]#λ,
   M <: HList,
@@ -16,9 +23,8 @@ package object dynamodb {
   Primary
   ](a: A) {
 
-    // todo remove?
+    //TODO: remove?
     val optionalRangeKey = RangeKey[Int]("rangeKeyCheat")
-
     def as[B](implicit entityGen: Generic.Aux[B, M]
               , zipper: Zip.Aux[A :: M :: HNil, N]
               , collectPrimaryKey: CollectFirst.Aux[A, HlistHelper.findPrimaryKey.type, PrimaryKey[Primary]]

src/test/scala/com/onzo/dynamodb/codec/ArbitraryInstances.scala

@@ -7,15 +7,14 @@ import org.scalacheck.{Arbitrary, Gen}
 
 trait ArbitraryInstances {
   private[this] def genBool: Gen[AttributeValue] = Arbitrary.arbBool.arbitrary.map(new AttributeValue().withBOOL(_))
-
+  
   private[this] def genString: Gen[AttributeValue] = Arbitrary.arbString.arbitrary.map(new AttributeValue().withS(_))
-
+  
   private[this] def genNumber: Gen[AttributeValue] = Arbitrary.arbLong.arbitrary.map(n => new AttributeValue().withN(n.toString))
-
+  
   private[this] def genAttributeValue: Gen[AttributeValue] = Gen.const(new AttributeValue())
-
+  
   implicit def arbitraryUUID: Arbitrary[UUID] = Arbitrary(Gen.uuid)
-
+  
   implicit def arbitraryAttributeValue: Arbitrary[AttributeValue] = Arbitrary(genAttributeValue)
-
 }