1

I am refactoring some scala code and I am having problems with a while loop. The old code was:

for (s <- sentences){
// ...
   while (/*Some condition*/){
      // ...
      function(trees, ...)
   }
}

I've have translated that code into this one, using foldLeft to transverse sentences:

sentences./:(initialSeed){
 (seed, s) =>
     // ...
     // Here I've replaced the while with other foldleft 
     trees./:(seed){
         (v, n) =>
             // ....
             val updatedVariable = function(...., v)
     }
}

Now, It may be the case that I need to stop transversing trees (The inner foldLeft before it is transverse entirely, for that I've found this question:

Abort early in a fold

But I also have the following problem:

As I transverse trees, I need to accumulate values to the variable v, function takes v and returns an updated v, called here updatedVariable. The problem is that I have the feeling that this is not a proper way of coding this functionality.

Could you recommended me a functional/immutable way of doing this?

NOTE: I've simplified the code to show the actual problem, the complete code is this:

val trainVocabulart = sentences./:(Vocabulary()){
  (vocab, s) =>
    var trees = s.tree
    var i = 0
    var noConstruction = false

    trees./:(vocab){
      (v, n) =>
        if (i == trees.size - 1) {
          if (noConstruction) return v
          noConstruction = true
          i = 0
        } else {
          // Build vocabulary
          val updatedVocab = buildVocabulary(trees, v, i, Config.LeftCtx, Config.RightCtx)

          val y = estimateTrainAction(trees, i)

          val (newI, newTrees) = takeAction(trees, i, y)

          i = newI
          trees = newTrees

          // Execute the action and modify the trees
          if (y != Shift)
            noConstruction = false

          Vocabulary(v.positionVocab ++ updatedVocab.positionVocab,
            v.positionTag ++ updatedVocab.positionTag,
            v.chLVocab ++ updatedVocab.chLVocab,
            v.chLTag ++ updatedVocab.chLTag,
            v.chRVocab ++ updatedVocab.chRVocab,
            v.chRTag ++ updatedVocab.chRTag)
        }
      v
    }
}

And the old one:

for (s <- sentences) {
  var trees = s.tree
  var i = 0
  var noConstruction = false
  var exit = false
  while (trees.nonEmpty && !exit) {
    if (i == trees.size - 1) {
      if (noConstruction) exit = true
      noConstruction = true
      i = 0
    } else {
      // Build vocabulary
      buildVocabulary(trees, i, LeftCtx, RightCtx)

      val y = estimateTrainAction(trees, i)

      val (newI, newTrees) = takeAction(trees, i, y)

      i = newI
      trees = newTrees

      // Execute the action and modify the trees
      if (y != Shift)
        noConstruction = false
    }
  }
}
Community
  • 1
  • 1
Alejandro Alcalde
  • 5,990
  • 6
  • 39
  • 79
  • 1
    Here's an idea: you can use either a simple recursion or construct a stream that will be lazily evaluated and terminate upon some condition. I would write a real piece of code, but it's kinda hard without the real data. Is the code publicly available? Can you consider sharing? It's much easier to give refactoring advice if you can lay your hands on some working code (at least in this case:) ) – tkroman Nov 03 '16 at 18:06
  • Of course, the whole project is at https://github.com/algui91/NLP_Dependency_Parsing, this piece of code is at `src/main/scala/com/elbauldelprogramador/nlp/parser/SVMParser.scala` – Alejandro Alcalde Nov 03 '16 at 18:08

1 Answers1

1

1st - You don't make this easy. Neither your simplified or complete examples are complete enough to compile.

2nd - You include a link to some reasonable solutions to the break-out-early problem. Is there a reason why none of them look workable for your situation?

3rd - Does that complete example actually work? You're folding over a var ...

trees./:(vocab){

... and inside that operation you modify/update that var ...

trees = newTrees

According to my tests that's a meaningless statement. The original iteration is unchanged by updating the collection.

4th - I'm not convinced that fold is what you want here. fold iterates over a collection and reduces it to a single value, but your aim here doesn't appear to be finding that single value. The result of your /: is thrown away. There is no val result = trees./:(vocab){...

One solution you might look at is: trees.forall{ ... At the end of each iteration you just return true if the next iteration should proceed.

jwvh
  • 50,871
  • 7
  • 38
  • 64
  • Thanks for your answer, actually, the value returned by `trees./:` is assigned to `trainVocabulart` – Alejandro Alcalde Nov 03 '16 at 18:45
  • Ah, so it is. I missed that. The SO code window isn't long enough to allow viewing the entire example without scrolling. – jwvh Nov 03 '16 at 19:03
  • It is writing a tail recursive function a better way of approaching this problem? – Alejandro Alcalde Nov 03 '16 at 19:09
  • It could well be. [Erik Meijer](https://en.wikipedia.org/wiki/Erik_Meijer_%28computer_scientist%29) calls recursion the GOTO of functional programming, but it does offer more complete control over iteration/termination as well as which elements to evaluate and which ones to ignore. – jwvh Nov 03 '16 at 19:39