From f20c57399d2205eaede2538d2c42fc57f10afccc Mon Sep 17 00:00:00 2001
From: Samouwow <sam.moran73@gmail.com>
Date: Fri, 28 Jun 2024 02:45:47 +0000
Subject: [PATCH] * Added review suggestions

---
 src/runtime/forester.rs      | 21 ++++++++-------------
 src/runtime/forester/flow.rs | 25 ++++++++++++-------------
 2 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/src/runtime/forester.rs b/src/runtime/forester.rs
index c13c8c0..57e1575 100644
--- a/src/runtime/forester.rs
+++ b/src/runtime/forester.rs
@@ -230,17 +230,15 @@ impl Forester {
                                             debug!(target:"flow[run]", "tick:{}, {tpe}. stay with the new state: {}",ctx.curr_ts(),&ns);
                                             ctx.new_state(id, ns)?;
                                         }
-                                        FlowDecision::Halt { .. } => {
-                                            unreachable!("A child returning running should not trigger a flow node halt decision.");
+                                        FlowDecision::Halt(..) => {
+                                            return Err(RuntimeError::Unexpected("A child returning running should not trigger a flow node halt decision.".to_string()));
                                         }
                                     }
                                 }
                             }
                             // Halting must be an atomic process, it is never spread over multiple ticks so should never be seen in this flow.
                             RNodeState::Halting(_) => {
-                                unreachable!(
-                                    "A flow node child should never return a state of Halting."
-                                );
+                                return Err(RuntimeError::Unexpected("A child returning running should not trigger a flow node halt decision.".to_string()));
                             }
                             // child is finished, thus the node needs to make a decision how to proceed.
                             // this stage just updates the status and depending on the status,
@@ -265,16 +263,12 @@ impl Forester {
                                         debug!(target:"flow[run]", "tick:{}, {tpe}. The '{}' is finished as {}, the new state: {}. Stay at this node. ",ctx.curr_ts(),child,s, &ns);
                                         ctx.new_state(id, ns)?;
                                     }
-                                    FlowDecision::Halt {
-                                        new_state,
-                                        halting_child_cursor,
-                                    } => {
+                                    FlowDecision::Halt(new_state, halting_child_cursor) => {
                                         // Normally we would fall through to Failure or Success next tick (e.g. Stay decision), but we need to pass control to the child so it can halt properly.
                                         // The current node can continue as normal once the child is halted.
                                         ctx.new_state(id, new_state.clone())?;
                                         // Force the state of the child to be halting, so it will interrupt itself on the next tick.
-                                        let halting_child_id =
-                                            children[halting_child_cursor as usize];
+                                        let halting_child_id = children[halting_child_cursor];
                                         debug!(target:"flow[run]", "tick:{}, {tpe}. The '{}' is finished as {}, the new state: {}. Halting child '{}'. ",ctx.curr_ts(),child,s, &new_state, &halting_child_id);
                                         ctx.force_to_halting_state(halting_child_id)?;
                                         ctx.push(halting_child_id)?;
@@ -346,9 +340,10 @@ impl Forester {
                         }
                         // Halting must be an atomic process, it is never spread over multiple ticks so should never be seen in this flow.
                         RNodeState::Halting(_) => {
-                            unreachable!(
+                            return Err(RuntimeError::Unexpected(
                                 "A decorator node child should never return a state of Halting."
-                            );
+                                    .to_string(),
+                            ));
                         }
                         // child is finished, thus the node needs to make a decision how to proceed.
                         // this stage just updates the status and depending on the status,
diff --git a/src/runtime/forester/flow.rs b/src/runtime/forester/flow.rs
index 0abc6e0..5767cad 100644
--- a/src/runtime/forester/flow.rs
+++ b/src/runtime/forester/flow.rs
@@ -5,6 +5,8 @@ use crate::runtime::{RtResult, RuntimeError, TickResult};
 use std::cmp::max;
 use FlowDecision::{Halt, PopNode, Stay};
 
+type HaltingChildCursor = usize;
+
 // current child
 pub const CURSOR: &str = "cursor";
 // the child len
@@ -141,10 +143,10 @@ pub fn finalize(
                         if running > cursor {
                             // This failure result needs to interrupt the running child.
                             // Note non-reactive sequences will always have running == p_cursor == cursor, so this will be unreachable for them.
-                            return Ok(Halt {
-                                new_state: RNodeState::Failure(run_with(args, cursor, len)),
-                                halting_child_cursor: running,
-                            });
+                            return Ok(Halt(
+                                RNodeState::Failure(run_with(args, cursor, len)),
+                                running as usize,
+                            ));
                         }
                     }
 
@@ -231,10 +233,10 @@ pub fn finalize(
                         if running > cursor {
                             // This success result needs to interrupt the running child.
                             // Note non-reactive fallbacks will always have running == p_cursor == cursor, so this will be unreachable for them.
-                            return Ok(Halt {
-                                new_state: RNodeState::Success(run_with(args, cursor, len)),
-                                halting_child_cursor: running,
-                            });
+                            return Ok(Halt(
+                                RNodeState::Success(run_with(args, cursor, len)),
+                                running as usize,
+                            ));
                         }
                     }
 
@@ -342,7 +344,7 @@ pub fn halt(flow_type: &FlowType, tick_args: RtArgs) -> (RNodeState, Option<usiz
             let running_child_cursor = tick_args
                 .find(RUNNING_CHILD.to_string())
                 .and_then(RtValue::as_int)
-                .and_then(|v| Some(v as usize));
+                .map(|v| v as usize);
             let mut args = tick_args.remove(RUNNING_CHILD);
 
             // MSequence needs to keep its position, but other nodes don't.
@@ -365,10 +367,7 @@ pub fn halt(flow_type: &FlowType, tick_args: RtArgs) -> (RNodeState, Option<usiz
 pub enum FlowDecision {
     PopNode(RNodeState),
     Stay(RNodeState),
-    Halt {
-        new_state: RNodeState,
-        halting_child_cursor: i64,
-    },
+    Halt(RNodeState, HaltingChildCursor),
 }
 
 fn replace_child_state(args: RtArgs, idx: usize, v: i64) -> RtArgs {