4
\$\begingroup\$

I'm currently working on a Scala HTTP library, fetch, mostly because I have yet to find one that suits all my needs.

As part of this library, I need a generic key / value store, which I intend to use both for request / response headers and URL query parameters. This store uses type classes for value parsing and formatting, which makes it both type-safe and convenient to use: ETags, for example, can be manipulated through an ETag class, but I do not need the key / value store to be aware that etags even exist.

This is what I have come up with:

trait ValueReader[S, T] {
  def read(value: S): Try[T]
}

trait ValueWriter[S, T] {
  def write(value: S): Option[T]
}

class KeyValueStore[T](val values: Map[String, T] = Map()) {
  def apply[S](name: String)(implicit reader: ValueReader[T, S]): S =
    reader.read(values(name)).get

  def getOpt[S](name: String)(implicit reader: ValueReader[T, S]): Option[S] = for {
    raw    <- values.get(name)
    parsed <- reader.read(raw).toOption
  } yield parsed

  def get[S](name: String)(implicit reader: ValueReader[T, S]): Option[Try[S]] = values.get(name) map reader.read

  def set[S](name: String, value: S)(implicit writer: ValueWriter[S, T]): KeyValueStore[T] =
    writer.write(value).fold(this)(set(name, _))

  def set(name: String, value: T): KeyValueStore[T] = new KeyValueStore(values + (name -> value))

  def setIfEmpty[S](name: String, value: S)(implicit writer: ValueWriter[S, T]): KeyValueStore[T] =
      if(contains(name)) this
      else               set(name, value)

  def remove(name: String): KeyValueStore[T] =
      if(contains(name)) new KeyValueStore[T](values - name)
      else               this

  def contains(name: String): Boolean = values.contains(name)
}

See this class for sample implementations of readers and writers.

I'd be very happy for this code to be criticised and improvements to be suggested, especially in the following areas:

  • variance: I will probably end up making this covariant in T. Is there any reason not to do so?
  • I've included three "accessor" methods: apply (which throws), getOpt (safe but doesn't let the caller know what went wrong) and get (safe, granular, but heavier). Is there a standard pattern for this, or is what I'm doing considered acceptable / idiomatic?
  • I'm pretty sure I'm not the first person to need this kind of class, are there standard methods that are expected and I'm not providing?
  • what's the general consensus on "operator-like" methods, such as - for remove or + for add?
\$\endgroup\$
  • \$\begingroup\$ That link for your implementation class gets a 404. \$\endgroup\$ – Bob Dalgleish May 28 '14 at 21:03
  • \$\begingroup\$ Sorry, my fault, I'd failed to pin the link to a specific git commit. It might be less than useful though, as I experimented quite a bit with this design since I initially asked the question... \$\endgroup\$ – Nicolas Rinaudo May 28 '14 at 21:17
2
\$\begingroup\$

I don't see how to understand the KeyValueStore without understanding the ValueFormat implementation you reference. You don't have any substantial comments in either implementation, including "why". Their mutual dependency makes it very difficult to review either of them.

Readability issues:

  1. You use ValueReader[S,T] in the KeyValueStore and ValueReader[T] in the ValueFormat -- this caused many minutes of confusion and blind paths.
  2. Similarly, you define trait ValueReader[S,T] and then you immediately turn around and use ValueReader[T,S] in the body of KeyValueStore. Swapping the parameters causes more confusion.
\$\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.