2
\$\begingroup\$

I've been working on an small API wrapper for the GitHub API using Scala (full source on GitHub). I'm still very new to the language, so I was hoping to get some feedback about a couple of design decisions.


The main class is APIClient. Within this class, a query for a repository takes the following steps:

  1. Make API call and parse result as JSON
  2. Pass JSON into a RepositoryResult class, which defines a .toRepository method to extract the fields for a Repository object
  3. Return the result of .toRepository

Here is the relevant code that accomplishes this:

APIClient: The main class used to interface between the client and the API.

class APIClient(private val authToken: Option[String]) {
  val baseUrl = "https://api.github.com"
  def this() = this(None)

  /* Queries the GitHub API at the specified endpoint. Provides OAuth token, if available. */
  private def query(target: String, params: Seq[(String, String)] = Seq(), headers: Seq[(String, String)] = Seq(), acceptFailure: Boolean = false): String = {
    val url = baseUrl + target
    val authParams: Seq[(String, String)] = authToken match {
      case Some(token) => ("access_token", token) +: params
      case None => params
    }
    val response = Http(url).params(authParams).headers(headers).asString
    if (!response.isSuccess) {
      if (acceptFailure) {
        ""
      } else {
        val result = JSON.parseFull(response.body).get.asInstanceOf[Map[String, Any]]
        sys.error(s"Error code ${response.code} when querying $url: ${result("message")}")
      }
    }
    else {
      response.body
    }
  }

  /** 
    * Parses a JSON string to either a List[Any] (in the case of a JSON array) or Map[String, Any] (in the case of a
    * JSON object) and returns the result wrapped an Either.
    */
  private def parse(json: String): Either[List[Any], Map[String, Any]] = {
    JSON.parseFull(json) match {
      case Some(parsed) => parsed match {
        case list: List[_] => Left(list)
        case map: Map[_,_] => Right(map.asInstanceOf[Map[String, Any]])
      }
      case None => sys.error(s"Unable to parse JSON.\n$json")
    }
  }

  /**
    * Takes a repository as a Map and returns a Repository object. If languages or the README are requested, makes
    * additional API calls to retrieve them.
    */
  private def generateRepo(repo: Any, withLanguages: Boolean, withReadMe: Boolean): Repository = {
    repo match {
      case result: Map[_,_] =>
        val stringMap = result.asInstanceOf[Map[String, Any]]
        val fullName = stringMap("full_name").asInstanceOf[String]
        val languages: Map[String, Long] = if (withLanguages) getLanguages(fullName) else Map[String, Long]()
        val readMe: String = if (withReadMe) getReadMe(fullName) else ""
        RepositoryResult(stringMap, readMe, languages).toRepository
    }
  }

  /**
    * Takes a sequence of repositories as Maps and returns a sequence of Repositories. If languages or the README are
    * requested, makes additional API calls to retrieve them.
    */
  private def generateRepos(repos: List[Any], withLanguages: Boolean, withReadMe: Boolean): Seq[Repository] = repos.map(generateRepo(_,withLanguages,withReadMe))

  // Requests repository information for a specific user and repository name.
  def getRepo(user: String, repo: String, withLanguages: Boolean = false, withReadMe: Boolean = false): Repository = {
    val json = query(s"/repos/$user/$repo")
    val parsed = parse(json).right.get
    generateRepo(parsed, withLanguages, withReadMe)
  }

  // Requests repository information for a user and generates a sequence of Repository objects.
  def getRepos(user: String, withLanguages: Boolean = false, withReadMe: Boolean = false): Seq[Repository] = {
    val json = query(s"/users/$user/repos")
    val parsed = parse(json).left.get
    generateRepos(parsed, withLanguages, withReadMe)
  }

  // Requests language information for a given repository (specified by a full name, e.g. "username/repository".
  def getLanguages(repo: String): Map[String, Long] = {
    val json = query(s"/repos/$repo/languages")
    val parsed = parse(json).right.get
    parsed.transform((str:String, dbl:Any) => dbl.asInstanceOf[Double].toLong)
  }

  // Requests the README of a repository.
  def getReadMe(repo: String): String = {
    query(s"/repos/$repo/readme", headers = Seq("Accept" -> "application/vnd.github.VERSION.raw"), acceptFailure = true)
  }

  // Requests information for a user and generates a User object.
  def getUser(user: String): User = {
    val json = query(s"/users/$user")
    val result = UserResult(parse(json).right.get)
    result.toUser
  }

  // Performs a repository search and returns the sequence of repositories retrieved.
  def searchRepos(searchQuery: SearchQuery, withLanguages: Boolean = false, withReadMe: Boolean = false): Seq[Repository] = {
    val json = query("/search/repositories", searchQuery.toParams)
    val result = parse(json).right.get
    val repoList = result("items").asInstanceOf[List[Any]]
    generateRepos(repoList, withLanguages, withReadMe)
  }

  // Performs a repository search and returns the sequence of repositories retrieved.
  def searchRepos(searchQuery: String): Seq[Repository] = searchRepos(SearchQuery(searchQuery))
}

object APIClient {
  val dateFormatter = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")

  /**
    * Attempts to read the first line of a text file "res/token.txt" for an OAuth access token.
    * This token can be generated from your GitHub account under the developer settings.
    * Using a token increases your rate limit from 60/hr to 5000/hr.
    */
  def getToken: Option[String] = Try(Some(Source.fromFile("res/token.txt").getLines().next())).getOrElse(None)
}

APIResult: The base class for API call results, providing useful methods for parsing the JSON response.

class APIResult(result: Map[String, Any]) {
  def getString(key: String): String = result(key).asInstanceOf[String]
  def getInt(key: String): Int = result(key).asInstanceOf[Double].toInt
  def getBoolean(key: String): Boolean = result(key).asInstanceOf[Boolean]
  def getDate(key: String): Date = APIClient.dateFormatter.parse(getString(key))
}

RepositoryResult: The class used to hold the JSON map and parse the relevant fields from JSON.

case class RepositoryResult(result: Map[String, Any], readMe: String, languages: Map[String, Long]) extends APIResult(result) {
  def toRepository: Repository = {
    val url = getString("url")
    val name = getString("name")
    val id = getInt("id")
    val description = getString("description")
    val createdAt = getDate("created_at")
    val updatedAt = getDate("updated_at")
    val pushedAt = getDate("pushed_at")
    val stars = getInt("stargazers_count")
    val watchers = getInt("watchers_count")
    val hasPages = getBoolean("has_pages")
    val forks = getInt("forks_count")
    val defaultBranch = getString("default_branch")
    Repository(url, name, id, description, readMe, languages, createdAt, updatedAt, pushedAt, stars, watchers, hasPages, forks, defaultBranch)
  }
}

Repository: Case class for repository fields.

case class Repository(url: String, name: String, id: Int, description: String,
  readMe: String, languages: Map[String, Long], createdAt: Date, updatedAt: Date, 
  pushedAt: Date, stars: Int, watchers: Int, hasPages: Boolean, forks: Int, 
  defaultBranch: String)

Another component to the client is searches. I used an immutable sort-of builder pattern to make it easy to construct SearchQuery objects (which can translate to valid search strings).

Qualifier: Class to represent search query qualifiers.

case class Qualifier(left: String, right: String, negate: Boolean = false) {
  override def toString: String = (if (negate) "-" else "") + left + ":\"" + right + "\""
}

SearchQuery: Class to help build search queries. Can be converted to a valid search query string.

/** Consult https://developer.github.com/v3/search/ and https://help.github.com/articles/search-syntax/ for details on
  * parameters and qualifiers.
  */
case class SearchQuery(query: String, qualifiers: Map[String,Qualifier] = Map(), parameters: Map[String,String] = Map()) {

  /**
    * Returns the parameters for the query
    */
  def toParams: Seq[(String, String)] = {
    ("q", query + qualString) +: parameters.toSeq
  }

  /**
    * Adds a qualifier to the query. Will overwrite other qualifiers of the same type.
    */
  def qualify(qual: Qualifier): SearchQuery = copy(qualifiers = qualifiers + (qual.left -> qual))
  def qualify(key: String, value: String): SearchQuery = qualify(Qualifier(key, value))
  def exclude(key: String, value: String): SearchQuery = qualify(Qualifier(key, value, negate = true))

  /**
    * Adds a parameter to the query. Will overwrite other parameters with the same name.
    */
  def addParam(param: String, value: String): SearchQuery = {
    assert(param != "q", "Cannot overwrite the search keyword parameter")
    copy(parameters = parameters + (param -> value))
  }
  def sortBy(sort: String): SearchQuery = addParam("sort", sort)
  def orderBy(order: String): SearchQuery = addParam("order", order)
  def getPage(pageNumber: Int): SearchQuery = addParam("page", pageNumber.toString)
  def perPage(pageSize: Int): SearchQuery = addParam("per_page", pageSize.toString)

  /**
    * Concatenates all the qualifiers of the query.
    * The qualifiers are the options included in the "q=_" parameter of the query, and are different from the other
    * parameters (e.g. sort, order)
    */
  def qualString: String = {
    qualifiers.foldLeft("")(
      (curr: String, pair: (String, Qualifier)) => {
        val (_, qual) = pair
        curr + " " + qual.toString
      }
    )
  }
}

I would like to be able to subclass SearchQuery with specific builder methods for different search types. However, qualify (and addParam) have result type of SearchQuery (and not something more specific like RepositorySearchQuery), so I would be unable to chain methods together. Downcasting seems like code smell to me. Is there a better way to accomplish this?

\$\endgroup\$
  • \$\begingroup\$ You left a lot of code out, making the reviewing needlessly difficult. The maximum character limit is 65k characters, feel free to add the rest of your code. It would definitely improve your question. \$\endgroup\$ – Mast Feb 5 '17 at 14:25
  • \$\begingroup\$ Sorry, I was aiming for conciseness, but I guess more code is better on Code Review. I've added more code to the post. \$\endgroup\$ – MattDs17 Feb 5 '17 at 19:51
  • \$\begingroup\$ Nice job including all relevant code :-) I'll gladly vote to reopen. \$\endgroup\$ – t3chb0t Feb 5 '17 at 20:35
2
+50
\$\begingroup\$

Case Classes

Some of the occurrences of case classes are overused. I doubt that RepositoryResult or UserResult could be useful in matchers, because they look like intermediate utility entities providing conversion to target Repository and User instances. This transformation can be done through a redefinition of apply function of companion objects.

APIResult super-class also introduces a rigid structure and can be transformed into a trait:

trait APIResult {

  def getString(key: String)(implicit apiResponse: Map[String, Any]): String = apiResponse(key).asInstanceOf[String]
  def getInt(key: String)(implicit apiResponse: Map[String, Any]): Int = apiResponse(key).asInstanceOf[Double].toInt
  def getBoolean(key: String)(implicit apiResponse: Map[String, Any]): Boolean = apiResponse(key).asInstanceOf[Boolean]
  def getDate(key: String): Date = APIClient.dateFormatter.parse(getString(key))

}

The implicit apiResponse arg above allows us to reduce the number of references to pass from one object to another. Now we can introduce companion objects like following:

object Repository extends APIResult {

  def apply(apiResponse: Map[String, Any], 
            readMe: String, 
            languages: Map[String, Long]): Repository = {
    implicit val resp = apiResponse // avoids to repeat the arg in each subsequent call and scoped locally!
    val url = getString("url")
    val name = getString("name")
    ...
    val defaultBranch = getString("default_branch")
    Repository(url, name, id, description, readMe, languages, createdAt, updatedAt, pushedAt, stars, watchers, hasPages, forks, defaultBranch)
  }

}

APIClient.generateRepo(...) may now return directly Repository(stringMap, readMe, languages)

Java API

We are now in the era of java.time, so I'd recommend to avoid java.util.Date by all means. Just forget that the old Date objects exist, they are evil!

import java.time.LocalDateTime

object APIClient {

  private val dateFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'")

  def parseDate(strDate: String): LocalDateTime = LocalDateTime.parse(strDate, dateFormatter)
  // or even move this utility stuff into APIResult trait, closer to where it is used.
...
}

... with respective changes in Repository and User constructors.

Don't Mix Patterns

The current version of SearchQuery object has a double responsibility: it wraps search query parameters and serves as a builder for other SearchQuery instances.

Let's first extract from it the part responsible for wrapped parameters:

case class SearchQuery(query: String, 
                       qualifiers: Map[String,Qualifier] = Map(),
                       parameters: Map[String,String] = Map()) {

  def toParams: Seq[(String, String)] = {
    ("q", query + qualString) +: parameters.toSeq
  }

  def qualString: String = {
    qualifiers.foldLeft("")(
      (curr: String, pair: (String, Qualifier)) => {
        val (_, qual) = pair
        curr + " " + qual.toString
      }
    )
  }
}

This allows us to have more flexibility for the implementation of the builder part.

Returning a SearchQuery from each chained method in the builder looks like a waste: there will be many instances created uselessly. More traditional approach is to return the same instance of the builder object.

Since we suppose that there will be more than one builder, we can imagine an abstract layer which will do most part of the work:

abstract class QueryBuilder[B, Q] {

  protected var query: Option[String] = None
  protected val qualifiers: mutable.Map[String, Qualifier] = mutable.Map()
  protected val parameters: mutable.Map[String, String] = mutable.Map()

  // returns the concrete instance of the current builder
  protected def getBuilderInstance(): B

  // builds the target object
  def build(): Q

  def qualify(qual: Qualifier): B = {
    qualifiers(qual.left) = qual
    getBuilderInstance()
  }
  def qualify(key: String, value: String): B = qualify(Qualifier(key, value))
  def exclude(key: String, value: String): B = qualify(Qualifier(key, value, negate = true))

  def addParam(param: String, value: String): B = {
    require(param != "q", "Cannot overwrite the search keyword parameter")
    parameters(param) = value
    getBuilderInstance()
  }

  def sortBy(sort: String): B = addParam("sort", sort)
  def orderBy(order: String): B = addParam("order", order)
  def getPage(pageNumber: Int): B = addParam("page", pageNumber.toString)
  def perPage(pageSize: Int): B = addParam("per_page", pageSize.toString)

  def queryString(sq: String): B = {
    this.query = Option(sq)
    getBuilderInstance()
  }

}

The B generic is the concrete type implementing the builder. The Q generic is the target type to build.

Now, to have a builder for SearchQuery, the implementation is brief:

class SearchQueryBuilder extends QueryBuilder[SearchQueryBuilder, SearchQuery] {

  override protected def getBuilderInstance(): SearchQueryBuilder = this

  override def build(): SearchQuery = if (query.isDefined) SearchQuery(query.get, qualifiers.toMap, parameters.toMap)
                                      else throw new IllegalStateException("Query String must be defined.")

}

If we had another type of *Query-like object to build, for example:

case class RepositorySearchQuery(query: String) { }

Its builder would be very similar:

class RepositorySearchQueryBuilder extends QueryBuilder[RepositorySearchQueryBuilder, RepositorySearchQuery] {

  override protected def getBuilderInstance(): RepositorySearchQueryBuilder = this

  override def build(): RepositorySearchQuery = if (query.isDefined) RepositorySearchQuery(query.get)
                                                else throw new IllegalStateException("Query String must be defined.")

  // override other functions if necessary...

}

Chained calls are perfectly possible:

val sq: SearchQuery = new SearchQueryBuilder().queryString("test").addParam("k", "v").build()
val rsq: RepositorySearchQuery = new RepositorySearchQueryBuilder().queryString("test").addParam("k", "v").build()

I hope this helps :)

\$\endgroup\$

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.