4

I am exploring different ways of implementing a linked list in Rust as a learning project. In one particular place, I've got some code that works properly, but it makes multiple calls to unwrap--I am under the impression this is generally regarded as unsafe/poor style. I'd like to make it better.

Here are some relevant definitions, with some unimportant details elided. Note that it is a singly linked list, with owning next pointers. These definitions should all be straight-forward and skim-able; I'll separate out the interesting part for ease of reading.

type NodePtr<T> = Option<Box<Node<T>>>;
struct Node<T> {
    data: T,
    next: NodePtr<T>,
}
pub struct LinkedList<T> {
    head: NodePtr<T>,
}
impl<T> LinkedList<T> {
    pub fn pop_back(&mut self) -> Result<T, LinkedListError> {
        if self.head.is_none() {
            Err(LinkedListError { kind: LinkedListErrorKind::Empty })
        } else {
            Ok(LinkedList::pop_last_node(&mut self.head))
        }
    }
    // definition for pop_last_node coming up shortly...
}

In this particular implementation I'm experimenting with recursive functions, and this is my working version of pop_last_node.

fn pop_last_node(node_ref: &mut NodePtr<T>) -> T {
    match node_ref.as_ref().unwrap().next {
        None => {
            let old_tail = node_ref.take();
            old_tail.unwrap().data
        }
        _ => LinkedList::pop_last_node(&mut node_ref.as_mut().unwrap().next)
    }
}

This is working correctly, but since I'm doing this as a learning experiment, I wanted to see if I could cut down on the unwrap calls and use some more pattern matching. This part of the experiment did not go as well.

Here is my attempt to do so. Unfortunately, this version is much more verbose (and confusing!) than the original. I especially don't like the "fall through out of this scope before you can do anything" part, but I haven't been able to come up with an idea on how to make it better.

fn pop_last_node(node_ref: &mut NodePtr<T>) -> T {
    {
        let next_node = match node_ref.as_mut() {
            None => panic!("Error handling will go here..."),
            Some(node_box) => &mut node_box.next,
        };
        match *next_node {
            None => {
                // fall through to code below
            },
            _ => {
                return LinkedList::pop_last_node(next_node)
            },
        }
    }
    // no sense converting this to a match--the "None" case was already checked above
    node_ref.take().unwrap().data
}

And that's where I am now. The main question is this: Is there a less crazy way to write the pattern matching version? Are there significant ways to improve the clarity or idiomatic-ness of either version?

GrandOpener
  • 1,943
  • 1
  • 17
  • 25
  • 1
    I'm voting to close this question as off-topic because it better belongs on [Code Review](http://codereview.stackexchange.com/), which is for questions to "*Improve code that you wrote or maintain, through peer review.*" Please note that the submission guidelines on Code Review are a bit different, so double check them before just copy-and-pasting your question. – Shepmaster Jun 22 '15 at 14:55
  • I think it's borderline since there is a specific question implied in the question, but I won't argue. I've reposted at: http://codereview.stackexchange.com/questions/94408/recursively-pop-the-last-node-from-a-singly-linked-list There were some minor edits to fit Code Review better, and also it doesn't have the pattern-matching tag since that tag doesn't exist over there and I don't have enough rep to create it. – GrandOpener Jun 23 '15 at 03:06

1 Answers1

1

Due to the Box, pattern matching with boxes is messy on stable. If you are willing to use nightly until box patterns are stabilized, you can rewrite your pop_back function (instead of just the pop_last_node function):

pub fn pop_back(&mut self) -> Result<T, LinkedListError> {
    fn pop_last_node<T>(node: &mut NodePtr<T>) -> Option<T> {
        match node.take() {
            None => None,
            // is a leaf
            Some(box Node { next: None, data }) => Some(data),
            Some(mut this) => {
                // recurse
                let ret = pop_last_node(&mut this.next);
                // put this subnode back, since it's not a leaf
                *node = Some(this);
                ret
            }
        }
    }
    pop_last_node(&mut self.head).ok_or(LinkedListError {
        kind: LinkedListErrorKind::Empty
    })
}

Try it out in the PlayPen

oli_obk
  • 28,729
  • 6
  • 82
  • 98
  • 1
    At Shepmaster's behest, I've reposted this question on Code Review at http://codereview.stackexchange.com/questions/94408/recursively-pop-the-last-node-from-a-singly-linked-list . Please repost your answer there so I can give you credit for it. – GrandOpener Jun 23 '15 at 03:07