mbeckerle commented on code in PR #1433:
URL: https://github.com/apache/daffodil/pull/1433#discussion_r1956319030
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -125,8 +125,8 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
.groupBy {
_._1
}
- .mapValues {
- _.map(_._2)
+ .map { case (k, v) =>
+ k -> v.map(_._2)
Review Comment:
If you use "(" instead of "{" this can be a one-liner that is equally, or
more clear:
```
val eventMap = eventTuples.groupBy(_._1).view.mapValues(_.map(_._2)).toMap
```
If eventTuples is just a transient value, then it could be a MapView, not a
map, and then the need to call `.view` would go away.
You also could entirely elminate the need to modify any mapValues calls by
this kind of technique:
```
import scala.language.implicitConversions
// Define RichMap to add mapValues method
implicit class RichMap[K, V](val underlying: Map[K, V]) extends AnyVal {
def mapValues[V2](f: V => V2): Map[K, V2] =
underlying.view.mapValues(f).toMap
}
```
What I like about that is it would let leave the mapValues calls in place,
so we can revisit them. Once we convert them all into unrolled map code with
`case (k, v) => ...` they're harder to find syntactically.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -256,15 +256,16 @@ trait ElementBase
// Creates a Map[prefix, Set[NS]]. Duplicate NS's will be removed from
the
// Set, since it's a Set
- val bindingsGroupedByPrefix = allBindings.groupBy { _._1 }.mapValues {
_.map { _._2 } }
+ val bindingsGroupedByPrefix =
Review Comment:
You do not have to rewrite all these mapValues. I believe you can just
change `.mapValues` to `.view.mapValues`. This gives a `MapView`, which behaves
like a map, except you have to call `toMap` at the end of the chain of
operations at some point, possibly, unless a `MapView` will do the job
entirely. I think `MapView` is not serializable.
But I imagine you considered that already and decided to avoid the MapView
thing. If so, can you explain why?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -130,7 +131,7 @@ trait ResolvesScopedProperties extends FindPropertyMixin {
self: Term =>
// Important - use of stream here insures we don't lookup
// properties down the chain once we have them here.
//
- val str = sources.toStream.map { _.chainFindProperty(pname) }
+ val str = sources.to(LazyList).map { _.chainFindProperty(pname) }
Review Comment:
I had to look up this `.to(LazyList)` You are passing the object LazyList
which obeys Factory pattern. Nice generalization. First thing in Scala 2.13/3.x
that adds value. I'm actually a fan of this. :-)
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/ChoiceCombinator.scala:
##########
@@ -236,7 +236,7 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
})
- val dispatchBranchKeyMap = dispatchBranchKeyValueTuples.toMap.mapValues
{ gram =>
+ val dispatchBranchKeyMap = dispatchBranchKeyValueTuples.toMap.map { case
(k, gram) =>
Review Comment:
This feels like a case where the original mapValues was clearer (because I
don't have to wonder what k is), and calling `.view.mapValues` and then
`.toMap` later would be clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
