1
\$\begingroup\$

I have the following code, it takes a list of maps representing bands and transforms the band properties via the pipeline_each method.

As you can see in the code, I am dealing with a lot of Options being returned when I modify the band data in the map - I am using an immutable map object and therefore need to return a new map each time the function is called.

I am also not happy about the type of the bands data structure being a List[Map[String, Any]] - particularly the Any part because I need to convert the values I get out of the maps to String types in order to modify them. I am stuck with this data structure though.

How can I make this code better, specifically addressing my concerns above?

    object Exercise {

  val bands: List[Map[String, Any]] = List(Map("name" -> "sunset rubdown", "country" -> "UK", "active" -> true),
  Map("name" -> "women", "country" -> "Germany", "active" -> false),
  Map("name" -> "a silver mt. zion", "country" -> "Spain", "active" -> true))


  def set_canada_as_country(band: Map[String, Any]): Map[String, Any] = {
    Map("name" -> band.get("name").get, "country" -> "Canada", "active" -> band.get("active").get )
  }

  def strip_punctuation_from_name(band: Map[String, Any]): Map[String, Any] = {
    Map("name" -> band.get("name").get.toString.replace(".", ""), "country" -> band.get("country").get, "active" -> band.get("active").get )
    //toString above is kinda gross and could cause a class cast exception
    //do I have to handle all the case matching on some/none for all the above calls to get?
  }

  def capitalize_names(band: Map[String, Any]): Map[String, Any] = {
    Map("name" -> band.get("name").get.toString.toUpperCase, "country" -> band.get("country").get, "active" -> band.get("active").get )
    //same again - don't like the toString
  }


  def pipeline_each(data: List[Map[String, Any]], fnList: List[Map[String, Any] => Map[String, Any]]): List[Map[String, Any]] = {
    fnList.foldLeft(data) {(bandList, currFn) => bandList.map(currFn)}
  }

}

I execute the above code via the REPL like so:

Exercise.pipeline_each(Exercise.bands,
  List(
    Exercise.set_canada_as_country,
    Exercise.strip_punctuation_from_name,
    Exercise.capitalize_names))
\$\endgroup\$
1
\$\begingroup\$

You might consider using map on Option to more cleanly access the values.

  def set_canada_as_country(band: Map[String, Any]): Map[String, Any] = {
    band.get("name").map( name => band.get("active").map(active => 
      Map("name" -> name, "country" -> "Canada", "active" -> active))
  }

At this point, you could reasonable change the resulting type to Map[String, String].

Rather than using a list of functions, you might want to compose a new function using andThen:

Exercise.pipeline_each(Exercise.bands, Exercise.set_canada_as_country andThen
  Exercise.strip_punctuation_from_name andThen
  Exercise.capitalize_names)

That way you are only applying a single filtering function.

\$\endgroup\$
  • \$\begingroup\$ A couple of things - I don't see how it's reasonable to change the resulting type to Map[String, String]. Second thing - I probably don't want to hard-code the composed function because I may want to supply other functions that modify the bands later? \$\endgroup\$ – jcm Jul 16 '14 at 23:59
  • \$\begingroup\$ As I understood it, you used Map[String,Any] because Option kept cropping up, meaning you had to use toString. Also, you can compose functions however you want; andThen is a better composition method than List. \$\endgroup\$ – Bob Dalgleish Jul 17 '14 at 11:33
1
\$\begingroup\$

Even if your original data structure is bound to something generic like Map[String,Any], you can still convert it to something more workable by defining apply() and converting to a case class. At that point, you can make use of a lot of built in Scala features that your fellow developers will thank you for using.

First, create a case class that reflects the data:

case class Band(name: Option[String]=None, country:Option[String]=None, active:Option[Boolean]=None) {}

Next, create an 'apply' method that converts the map.

object Band {
    def apply(input: Map[String,Any]) = {
        Band(
            name = Try(input.get("name").asInstanceOf[String]).toOption,
            country = Try(input.get("country").asInstanceOf[String]).toOption,
            active = Try(input.get("active").asInstanceOf[Boolean]).toOption
        )
    }
}

Now, you can use standard case class operations and comprehensions:

Exercise.bands.map(Band(_))
    .map{ band => band.copy(country=Some("Canada"))}
    .map{ band => band.copy(name=band.name.map(_.replace(".","")))}
    .map{ band => band.copy(name=band.name.map(_.toUpperCase))}
\$\endgroup\$
0
\$\begingroup\$

Scala uses camelCaseNotation, not underscore_notation.

List[Map[String, Any]] is indeed too complex. And Any should be avoided at all costs. See what I did below. Note that the band name is very important (cannot be an Option) and is different from the other fields.

Options are necessary here since many values in the "database" might be missing.

As mentioned by Bob Dalgleish, your pipeline_each is unnecessary since you are just composing functions.

 object BandDatabase {
    type BandDatabase = Map[String, BandInfo]
    case class BandInfo(country: Option[String], active: Option[Boolean]) 

    val bands: BandDatabase = Map(
      "sunset rubdown" -> BandInfo(Some("UK"), Some(true)),
      "women" -> BandInfo(Some("Germany"), Some(false)),
      "a silver mt. zion" -> BandInfo(Some("Spain"), Some(true)),
      "fake band" -> BandInfo(None, None))

    def mapKeys[A, B, C](map: Map[A, B], f: A => C): Map[C, B] =
      map.toList.map { case ((a, b)) => (f(a), b) }.toMap

    def mapValues[A, B, C](map: Map[A, B], f: B => C): Map[A, C] =
      map.mapValues(f)

    def stripPunctuationFromBandName: BandDatabase => BandDatabase =
      mapKeys(_, (s: String) => s.replaceAll("[,.!;:]", ""))

    def capitalizeBandNames(bands: BandDatabase): BandDatabase =
      mapKeys(bands, (s: String) => s.toUpperCase())

    def setCountryForAll(bands: BandDatabase, country: Option[String]): BandDatabase =
      mapValues(bands, (bi: BandInfo) => BandInfo(country, bi.active))

    val clean: BandDatabase => BandDatabase =
      stripPunctuationFromBandName
        .andThen(capitalizeBandNames)
        .andThen(setCountryForAll(_, Some("Canada")))
  }
  import BandDatabase._

  println(clean(bands))
\$\endgroup\$
0
\$\begingroup\$

If all you want to do is update the value for one member of a map with a new value you can use the updated method and the rest of the fields will be automatically copied.

  def set_canada_as_country(band: Map[String, Any]): Map[String, Any] = {
    band.updated("country","Canada")
  } 

For the other two functions where you want to modify one value use transform and write a function that modifies the value.

  def strip_punctuation_from_name(band: Map[String, Any]): Map[String, Any] = {
    band.transform{ (key,value) =>
      key match {
        case "name" => value.toString.replace("."," ")
        case _ => value
      }
    }
  } 

You could do capitalize_names in the same way and then you would have no options left.

\$\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.