0
\$\begingroup\$

I have a case class that is recursive and looks like this:

case class Config(name: String, isEnabled: Boolean, elems: Map[String, MyCase])
case class MyCase(
 id: String,
 isActive: Boolean,
 elems: Option[Map[String, MyCase]])

Where the Config contains the id of the MyCase entries contained as a Map. I have to iterate over this structure and come up with a Map that contains the parent child relations. Say for example., if I have the Config class represented as below (for simplicity, I have just given the id's):

Config(Map("5" -> myCase1, "6" -> myCase2))
5 - Config
 1
 1
 2 
 2
6
 1

where id's 5 and 6 are top level entries which in turn has a recursive structure. I have to now come up with a Map that contains the parent child relationship for the id's. So for the case above, I will expect a Map that looks like:

Map(
 "5" -> Seq("5.1"),
 "5.1" -> Seq("5.1.1", "5.1.2"),
 "5.1.1" -> Seq.empty[String],
 "5.1.2" -> Seq.empty[String],
 "6" -> Seq("6.1"),
 "6.1" -> Seq.empty[String]
)

Notice how I append the parent id to the child.

Given a Seq of such Configs,

val configs = Seq(Config1, Config2)

this is what I have come up with:

def relationsFor(config: Config): Map[String, Seq[String]] = {
 def prepareChildren(parentId: String, acc: Map[String, Seq[String]], subElems: Map[String, MyCase]): Map[String, Seq[String]] = {
 subElems.collect {
 case (id, elem) if elem.isActive =>
 val newAcc = acc ++ Map(parentId -> subElems.keys.map(subElemId => s"$parentId.$subElemId").toSeq)
 if (elem.elems.isDefined) {
 val newParentId = s"$parentId.$id"
 val newAcc1 = newAcc ++ Map(s"$parentId.$id" -> elem.elems.get.keys.map(subElemId => s"$newParentId.$subElemId").toSeq)
 prepareChildren(s"$parentId.$id", newAcc1, elem.elems.get)
 } else {
 acc ++ Map(s"$parentId.$id" -> Seq.empty[String])
 }
 }
 }.flatten.toMap
 configs.collect {
 case config if config.isEnabled =>
 config.elems.collect {
 case (parentId, elemCase) if elemCase.elems.isDefined =>
 prepareChildren(parentId, Map.empty[String, Seq[String]], elemCase.elems.get)
 case (parentId, _) =>
 Map(parentId -> Seq.empty[String])
 }
 }.flatten.flatten.toMap
 }

Could this be simplified ? I have already tested this and it works as expected, but I find it a little bit hard to understand. Can this be made much more elegant? I mean can the relationsFor method be made more elegant ?

Vogel612
25.5k7 gold badges59 silver badges141 bronze badges
asked Jul 1, 2016 at 12:32
\$\endgroup\$
2
  • \$\begingroup\$ Any suggestions? \$\endgroup\$ Commented Jul 1, 2016 at 16:35
  • 1
    \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers . \$\endgroup\$ Commented Jul 1, 2016 at 17:19

2 Answers 2

2
\$\begingroup\$

I tried do it a different way, when you asked this question on StackOverflow.. You find my version below, first I'll try to give you some feedback on your version :

  • I see you regularly use isDefined together with pattern matching or if / else. I would replace this by matching for Some / None or even better with a combination of map and getOrElse or with fold :

    o.map(f).getOrElse(b)
    o.fold(b)(f)
    

    These are more ideomatic ways to handle an Option and unwrap the Option for you where you want to use it (calling get on an Option is something you should try to minimize).

  • It seems like you could have reused newParentId in the places where you wrote s"$parentId.$id", this would mean that newParentId should be placed outside the if else.
  • I am not sure if you can change the signature of MyCase, but I am not sure what extra the Option brings for elems, but this may be moot if you want to differentiate between None and Map.empty[String, MyCase]], if you don't then no collection or an empty collections is pretty much the same.

Here's what I came up with :

def getMapWithChildIds(config: Config): Map[String, Seq[String]] = {
 def mkId(prefix: String, key: String): String = 
 if (prefix.isEmpty) key else prefix + "." + key
 def go(map: Map[String, MyCase], prefix: String): Map[String, Seq[String]] =
 map.flatMap { case (key, elem) => 
 val fullKey = mkId(prefix, key)
 // this will handle an unactive MyCase the same way as a MyCase with no elems
 Option(elem).filter(_.isActive).flatMap(_.elems).fold(
 Map(fullKey -> Seq.empty[String]))(
 submap => {
 val directSubKeys = submap.keys.map(mkId(fullKey, _)).toList
 // map entry for this key + entries for sub keys
 Map(fullKey -> directSubKeys) ++ go(submap, fullKey)
 })
 }
 go(config.elems, "")
}

Which gives for :

val noMap: Option[Map[String, MyCase]] = None
val config =
 Config("a", true,
 Map(
 "5" -> MyCase("id?5", true, 
 Some(Map(
 "1" -> MyCase("id?5.1", true,
 Some(Map(
 "1" -> MyCase("id?5.1.1", true, noMap),
 "2" -> MyCase("id?5.1.2", true, noMap)
 ))
 ),
 "2" -> MyCase("id?5.2", true, noMap)
 ))
 ),
 "6" -> MyCase("id?6", true, 
 Some(Map(
 "1" -> MyCase("id?6.1", true, noMap)
 ))
 )
 )
 )

The following result :

val map = getMapWithChildIds(config)
// Map[String,Seq[String]] = Map(5.2 -> List(), 5.1.2 -> List(), 5 -> List(5.1, 5.2), 5.1.1 -> List(), 6 -> List(6.1), 6.1 -> List(), 5.1 -> List(5.1.1, 5.1.2))
map.toList.sortBy(_._1).foreach(println)
// (5,List(5.1, 5.2))
// (5.1,List(5.1.1, 5.1.2))
// (5.1.1,List())
// (5.1.2,List())
// (5.2,List())
// (6,List(6.1))
// (6.1,List())
answered Jul 1, 2016 at 16:49
\$\endgroup\$
3
  • \$\begingroup\$ You are awesome! Your other example for my other question was a lot readable. Let me see if I can further simplify my version without those if else and isDefined on the Option! \$\endgroup\$ Commented Jul 1, 2016 at 16:53
  • \$\begingroup\$ The reason why I need the extra Option for the elems is that I'm reading this case class from JSON and there might or might not be this nesting for some elements \$\endgroup\$ Commented Jul 1, 2016 at 17:00
  • \$\begingroup\$ Have a look at my EDIT. I tried to come up with a simpler version, but just not sure if it would work. I could not test it right now. Will do it perhaps later. I really appreciate your help! \$\endgroup\$ Commented Jul 1, 2016 at 17:13
0
\$\begingroup\$

Here is a much better version, but it does not give me exactly what I want:

case class Config(name: String, isEnabled: Boolean, elems: Map[String, MyCase])
case class MyCase(
 id: String,
 isActive: Boolean,
 elems: Option[Map[String, MyCase]])
def prepareChilds(parentId: String, acc: Seq[(String, Seq[String])], elems: Seq[(String, MyCase)]): Seq[(String, Seq[String])] = {
 elems.foldLeft(acc) {(newAcc, elem) =>
 val (assetId, assetConfig) = elem
 if (assetConfig.elems.isDefined) {
 prepareChilds(
 s"$parentId.$assetId",
 newAcc :+ (parentId, assetConfig.elems.get.filter(_._2.isActive).keys.toSeq),
 assetConfig.elems.get.filter(_._2.isActive).toSeq)
 } else {
 newAcc :+ (parentId, Seq.empty[String])
 }
 }
}
val noMap: Option[Map[String, MyCase]] = None
val config =
 Config("a", true,
 Map(
 "5" -> MyCase("5", true, 
 Some(Map(
 "1" -> MyCase("5.1", true,
 Some(Map(
 "1" -> MyCase("5.1.1", true, noMap),
 "2" -> MyCase("5.1.2", true, noMap)
 ))
 ),
 "2" -> MyCase("5.2", true, noMap)
 ))
 ),
 "6" -> MyCase("6", true, 
 Some(Map(
 "1" -> MyCase("6.1", true, noMap)
 ))
 )
 )
 )

For the following input:

val result = prepareChilds("5", Seq.empty[(String, Seq[String])], config.elems.toSeq)

I get the following result:

List((5,ArrayBuffer(1, 2)), 
 (5.5,ArrayBuffer(1, 2)), 
 (5.5.1,List()), 
 (5.5.1,List()), 
 (5.5,List()), 
 (5,ArrayBuffer(1)), 
 (5.6,List())
)
answered Jul 1, 2016 at 18:34
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.