refactor: use guard pattern on Option #19

theotchlx · 2025-11-28T00:54:09Z

The guard pattern sometimes makes the code easier to work with.

Note on this first commit, that the code went from implicit returns to explicit return statements.
The compiler actually throws an error, and clippy doesn't complain about needless_return.

This may be a missing feature in the Rust compiler.

theotchlx · 2025-11-28T00:56:49Z

@olivier-dj what do you think about this use of guards? It fits the example you gave in issue #16 , removing one layer of indentation and checking each Option one after the other, and it doesn't seem to add confusion either.

If that's right, I'll find more places that fit the pattern and update them.

olivier-dj · 2025-11-28T09:49:24Z

@olivier-dj what do you think about this use of guards? It fits the example you gave in issue #16 , removing one layer of indentation and checking each Option one after the other, and it doesn't seem to add confusion either.

If that's right, I'll find more places that fit the pattern and update them.

Looks perfect to me, I find it much more readable !

This commit is changes from running `cargo fmt`

theotchlx · 2025-12-06T03:00:51Z

Made the changes were it felt relevant.
Ready for review - I did my best to check that I did not affect business logic flow anywhere.

Feel free to point out where it doesn't fit.

Propagate None using the question mark operator instead of with a conditional.

Using the question mark operator to simplify the code.

In this particular case, the new guard pattern makes the code easier to work with. Note that the code went from implicit returns to explicit return statements. The compiler actually throws an error, and clippy doesn't complain about needless_return.

Make the code cleaner by using the guard pattern.

theotchlx · 2025-12-06T16:31:47Z

The first commit is diffs from running cargo fmt.
The next two commits are diffs from using the ? operator to simplify the code.
The following commits are diffs from using the guards pattern to simplify the code.

olivier-dj

Have you tested a bit the changes with cargo bench & the examples?

olivier-dj · 2025-12-06T19:36:16Z

src/contact_manager/seg.rs

+            let Some(tx_end) = self.get_tx_end(tx_start, bundle.size, free_seg.end) else {
+                continue;
+            };
+            let delay = Self::get_delay(tx_end, &self.delay_intervals);
+            return Some(ContactManagerTxData {
+                tx_start,
+                tx_end,
+                delay,
+                expiration: free_seg.end,
+                arrival: tx_end + delay,
+            });


hmm I am unsure this is better. For readability this is debatable, and does the compiler drop the extra control flow associated to the "continue" ? Just binary size concerns, this may add an additional goto statement. (I bet LLVM drops the overhead)

hm alright, I've dropped the commit

olivier-dj · 2025-12-06T19:39:00Z

src/contact_plan/from_asabr_lexer.rs

+                                match Self::add_node(
+                                    node,
+                                    &mut nodes,
+                                    &mut max_node_in_in_nodes,
+                                    &mut known_node_ids,
+                                    &mut known_node_names,
+                                ) {
+                                    Ok(_) => {}
+                                    Err(msg) => {
+                                        return Err(msg);
+                                    }


just Self::add_node(....)?; (with ?) should work no?

Right! Lol. I've edited the commit.

olivier-dj · 2025-12-06T19:50:04Z

src/pathfinding/node_parenting.rs

+                        };
+
+                        let mut push = false;
+                        if let Some(know_route_ref) =


By replacing the push variable logic below by an inline function, we should be able to get right of this if Some()

I pushed a commit that replaces

let mut push = false; if let Some(know_route_ref) = tree.by_destination[receiver.node.borrow().info.id as usize].clone() { let mut known_route = know_route_ref.borrow_mut(); if D::cmp(&route_proposition, &known_route) == Ordering::Less { known_route.is_disabled = true; push = true; } } else { push = true; } if push { let route_ref = Rc::new(RefCell::new(route_proposition)); tree.by_destination[receiver.node.borrow().info.id as usize] = Some(route_ref.clone()); priority_queue.push(Reverse(DistanceWrapper::new(route_ref))); }

with

if tree.by_destination[receiver.node.borrow().info.id as usize] .as_ref() .map_or(true, |known_route_ref| { let mut known_route = known_route_ref.borrow_mut(); if D::cmp(&route_proposition, &known_route) == Ordering::Less { known_route.is_disabled = true; true } else { false } }) { let route_ref = Rc::new(RefCell::new(route_proposition)); tree.by_destination[receiver.node.borrow().info.id as usize] = Some(route_ref.clone()); priority_queue.push(Reverse(DistanceWrapper::new(route_ref))); }

olivier-dj · 2025-12-06T20:03:12Z

src/routing/mod.rs

+                let Some(ptr) = first_hop_ptr else { continue };
+                let Some((_, rts)) = first_hops_map.get_mut(&ptr) else {


Maybe:

let (Some(a), Some(b)) = (oa, ob) else { return; };

What do you think of this pattern ?

I tried:

let (Some(ptr), Some((_, rts))) = (first_hop_ptr, first_hops_map.get_mut(&ptr)) else { continue; }

Would've been nice, however unfortunately ptr is out of scope in the right-hand side of the equal sign...

"cannot find value ptr in this scope" in .get_mut(&ptr)

olivier-dj · 2025-12-06T20:04:47Z

src/routing/mod.rs

+                    continue;
+                };
+                rts.push(current_route.clone());
            } else if let Some(next_route) = route_borrowed.next_for_destination.get(&dest) {


complicated to replace those?

Do you mean place the continue on the right? Yeah it's not very consistent as-is, but cargo fmt places the continue on a newline, due to the character limit..

theotchlx · 2025-12-29T17:12:29Z

I have run 3 benchmarks (just as in #18 (comment)) and get similar alternating results.

style: format codebase with cargo fmt

2b713ac

This commit is changes from running `cargo fmt`

theotchlx force-pushed the refactor/control-flow-guards branch 2 times, most recently from 061c456 to 43ecc78 Compare December 6, 2025 02:57

theotchlx marked this pull request as ready for review December 6, 2025 03:00

theotchlx added 6 commits December 6, 2025 17:29

refactor(routing/volcgr): propagate None in route_unicast()

d69e6cb

Propagate None using the question mark operator instead of with a conditional.

refactor(contact_manager/legacy): propagate None in schedule_tx()

f02e5ad

Using the question mark operator to simplify the code.

refactor(contact_plan): use guard pattern on Option

05f0434

refactor(routing): use guards on update multicast dest

142a171

refactor(routing/cgr): use guard in route_unicast()

b20ed5a

Make the code cleaner by using the guard pattern.

theotchlx force-pushed the refactor/control-flow-guards branch from 43ecc78 to 1ffb0c2 Compare December 6, 2025 16:29

olivier-dj reviewed Dec 6, 2025

View reviewed changes

theotchlx force-pushed the refactor/control-flow-guards branch from 1ffb0c2 to 1911d57 Compare December 26, 2025 22:17

theotchlx added 5 commits December 27, 2025 00:33

refactor(contact_plan): use guard pattern in asabr lexer parse()

95efd5e

refactor(pathfinding/hybrid_parenting): use guards in get_next()

4f407f2

refactor(pathfinding/node_parenting): use guards in get_next()

385d5d5

refactor(route_stage): use guards in schedule()

a7ba35a

refactor(route_stage): use guards in dry_run()

07b4058

theotchlx force-pushed the refactor/control-flow-guards branch from 1911d57 to 07b4058 Compare December 26, 2025 23:34

refactor(pathfinding/node_parenting): inline push variable logic

554ae38

theotchlx force-pushed the refactor/control-flow-guards branch from dfdb087 to 554ae38 Compare December 28, 2025 19:10

		let Some(ptr) = first_hop_ptr else { continue };
		let Some((_, rts)) = first_hops_map.get_mut(&ptr) else {

refactor: use guard pattern on Option #19

Are you sure you want to change the base?

refactor: use guard pattern on Option #19

Uh oh!

Conversation

theotchlx commented Nov 28, 2025

Uh oh!

theotchlx commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivier-dj commented Nov 28, 2025

Uh oh!

theotchlx commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotchlx commented Dec 6, 2025

Uh oh!

olivier-dj left a comment

Choose a reason for hiding this comment

Uh oh!

olivier-dj Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotchlx commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

theotchlx commented Nov 28, 2025 •

edited

Loading

theotchlx commented Dec 6, 2025 •

edited

Loading

olivier-dj Dec 6, 2025 •

edited

Loading

theotchlx commented Dec 29, 2025 •

edited

Loading