Enhancement

[sanjeevghimire]

created a akka http client to interact with nosql db api.

[johanandren]

Added some feedback!

[johanandren]

Don't create a separate actor system for this, instead, make this a class that takes the system and materializer as constructor parameters, and pass in the system of the actor where you use it.

[johanandren]

I'd call it a "CloudantClient" or possibly a "CloudantHttpClient" instead, as it isn't a generic HTTP client, but rather provides cloudant specific things on top of the http client.

[johanandren]

This is an internal method, not a part of the public API of this class, so make it private (or package private if you want to be able to write unit tests for it)

[johanandren]

That the cloudant client does a post/get is internal details which the calling code does not have anything to do with, I think you should raise the abstraction level one step and call these save and load. (Imagine the corresponding if the JDBC API would say sendBytesOverNetworkWithTcp rather than executeQuery)

I still think that accepting strings and producing strings is the wrong abstraction, but one step at a time I guess.

If it ends up with wanting to avoid parsing the JSON (which I think would be the right thing) for performance reasons I would still introduce a new class denoting that it is not any string but raw json, in that case it may make sense to never even parse it into a string but keep it as bytes. Something like case class RawJson(rawData: ByteString).

[johanandren]

Reading config is somewhat costly, so it makes sense to rather read these once and cache the values in fields of the class when constructing the instance, just like you do with the auth header.

[sanjeevghimire]

Would creating a immutable map like below would work?

class AppConfig(implicit val system: ActorSystem,
                implicit val materializer: ActorMaterializer ) {
  private var config = scala.collection.immutable.Map[String, String]()
  def load(): Unit ={
    val appConfig = ConfigFactory.load();
    config+= ("username" -> appConfig.getString("cloudant.username"))
   ...
  }
  def getConfig(key: String): String ={
    config(key)
  }
}

[johanandren]

Yes, you could use a map, but I think just using fields are more clear and allow you to keep separate types with the values rather than either forcing all configurable values to be strings or

Note that you should not load the configuration yourself, instead you should use it from the actor context or system, so:

class AppConfig(implicit system: ActorSystem, materializer: ActorMaterializer ) {
   // gives us a config just containing the things inside cloudant { ... } in the config file
  private def config = system.settings.config.getConfig("cloudant")
  // these are strings, as for example
  private val username = config.getString("username")
  private val resultsUrl = config.getString("get_results_url")
  // just an example, a boolean setting
  private val useHttps = config.getBoolean("use-https")
... the rest of the class ... 

[johanandren]

Ups, of course not private then if you are creating a class that other classes use to know the settings :)

[johanandren]

Inside of Akka (and the surrounding projects) we call such classes NnnnSettings and a pretty useful thing is to model them as case classes, like this:

object CloudantSettings {
  def apply(config: Config) = 
    CloudAntSettings(config.getString("username"), ... ... )
  def apply(system: ActorSystem) = 
    apply(system.settings.config.getConfig("cloudant"))
}
case class CloudantSettings(
  username: String, 
  password: String,
  resultsUrl: String,
  ...etc...
)

This makes it easy to create custom instances both programmatically and from config.

[johanandren]

Perfect use case for Scala pattern matching, would make this

documentType match {
  case DocumentType.TeamTable => tablesUrl
  case DocumentType.Results => resultsUrl
  case DocumentType.Fixtures => fixturesUrl
}

additionally changing DocumentType into an ADT (algebraic data type), like so:

sealed trait DocumentType
case object TeamTable extends DocumentType
case object Results extends DocumentType
case object Fixtures extends DocumentType

would then give you a compiler error if you at some future point added another document type but forgot to update the pattern match expression to cover all possible values.

[johanandren]

Seems like it can be deleted now

[johanandren]

This doesn't look right to me, you cannot just typecast a ByteString to a String, you should either Unmarshal a string above if possible, or you would have to .map(byteString => byteString.utf8String) to parse the bytes into a String