3
\$\begingroup\$

I am learning Rust by implementing a series of simple modules which I tend to use a lot in other languages. One of them describes simple methods done on intervals (union, intersection, etc). Here I define a method to split an interval into 2 at a given position.

use std::error::Error;

#[derive(PartialEq)]
#[derive(Debug)]
pub(crate) struct SEGTP {
    start: u32,
    end: u32,
}

impl Default for SEGTP {
    fn default() -> Self {
        SEGTP {
            start: u32::MAX,
            end: u32::MAX,
        }
    }
}

impl SEGTP {
    pub fn new(start:u32, end:u32) -> Result<Self, Box<dyn Error>>{
        if start > end{
            return Err("start > end".into());
        }
        Ok(Self {
            start,
            end,
        })
    }

    pub fn intersect(&self, s: &SEGTP) -> Option<SEGTP>{
        let start = self.start.max(s.start);
        let end = self.end.min(s.end);

        if start <= end {
            Some(SEGTP::new(start, end).unwrap())
        } else {
            None
        }
    }

    // split interval into 2 on position
    pub fn split_at(&self, pos: u32) -> Result<(Option<SEGTP>, Option<SEGTP>), Box<dyn Error>> {
        let res = match pos{
            p if p <= self.start => (None, Some(SEGTP::new(self.start,self.end)?)),
            p if p > self.end => (Some(SEGTP::new(self.start,self.end)?),None),
            _ => (Some(SEGTP::new(self.start,pos-1)?),Some(SEGTP::new(pos,self.end)?)),
        };
        Ok(res)
    }

    // split interval into 3 based on the intersection with another interval
    pub fn split_by(&self, s: &SEGTP) -> Result<(Option<SEGTP>,Option<SEGTP>,Option<SEGTP>),Box<dyn Error>>{
        let iseg = self.intersect(s);
        let lseg = self.split_at(s.start)?;
        let rseg = self.split_at(s.end)?;

        let (ls,is,rs) = (lseg, iseg, rseg);
        Ok((ls.0,is,rs.1))
    }
}

Does this implementation, with given return signature and usage of match statement instead of if seem reasonable or unnecessarily complicated? What would one change? Is there an advantage to using self.clone() over constructing a new struct and returning it?

I am a total novice and would like to develop better habits, but am struggling with when to use rust-specific syntax and expressions (Options, Results, error propagation, match, etc), vs more conventional techniques (if/else, assertions, simple return types, etc). Are there any complete app examples that are good to read for learning and to see how various techniques are being used? Or should I not bother and keep writing code until I get familiar enough to make more informed decisions?

\$\endgroup\$
2
  • \$\begingroup\$ There is no comment in this code to help me out; should I mentally pronounce this "SEGmenT Pointers" ? Also, it would help if you could describe the half-open interval you're trying to support, similar to python's range(a, b). \$\endgroup\$
    – J_H
    May 1, 2023 at 22:18
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ May 2, 2023 at 6:54

1 Answer 1

2
\$\begingroup\$
use std::error::Error;

#[derive(PartialEq)]
#[derive(Debug)]

You can combine derives onto a single line: #[derive(PartialEq, Debug)]

pub(crate) struct SEGTP {
    start: u32,
    end: u32,
}

Rust typically uses UpperCamelCase for typenames. I'd probably name this something like Segment just to make it really clear what it means.

I also wonder why you are using pub(crate) here.

impl Default for SEGTP {
    fn default() -> Self {
        SEGTP {
            start: u32::MAX,
            end: u32::MAX,
        }
    }
}

This looks pretty dubious to me. Default should create a sensible default, this looks instead to be some sort of invalid segment.

impl SEGTP {
    pub fn new(start:u32, end:u32) -> Result<Self, Box<dyn Error>>{

        if start > end{
            return Err("start > end".into());
        }

The first question is: is this error better as a panic! or a Result. It looks like an invariant that the calling code should guarantee is true. If so, dealing with an error return value may be more annoyance than its worth. You could simply do assert!(state > end) and then avoid returning errors at all.

You may like to read some discussion about when to panic!.

Alternately, perhaps this should really return an Option. Instead of returning an error when the segment is invalid, return None. I suspect that some of your calling code may be simpler in that case.

If you decide to use an Result what error type should you be using. In my opinion, Box<dyn Error> is a dubious choice. The problem is that it doesn't capture stack traces like the errors from anyhow or color-eyre and it also doesn't provide good ability to figure out what the error was and recover from it. Typically, application code gets good use out of something like anyhow or color-eyre which provide traces. But library code that get reused often has a specific error type for that module.

        Ok(Self {
            start,
            end,
        })
    }

    pub fn intersect(&self, s: &SEGTP) -> Option<SEGTP>{
        let start = self.start.max(s.start);
        let end = self.end.min(s.end);

        if start <= end {
            Some(SEGTP::new(start, end).unwrap())
        } else {
            None
        }
    }

    // split interval into 2 on position
    pub fn split_at(&self, pos: u32) -> Result<(Option<SEGTP>, Option<SEGTP>), Box<dyn Error>> {

If I follow to logic correctly, its not possible for this function to actually return an error. That's the kind of case where its better not to return a result type. Since the invariants guarantee the lack of error, I'd just unwrap().

        let res = match pos{

You don't really get any benefit from match here, I'd just use if.

            p if p <= self.start => (None, Some(SEGTP::new(self.start,self.end)?)),
            p if p > self.end => (Some(SEGTP::new(self.start,self.end)?),None),
            _ => (Some(SEGTP::new(self.start,pos-1)?),Some(SEGTP::new(pos,self.end)?)),
        };
        Ok(res)
    }

    // split interval into 3 based on the intersection with another interval
    pub fn split_by(&self, s: &SEGTP) -> Result<(Option<SEGTP>,Option<SEGTP>,Option<SEGTP>),Box<dyn Error>>{
        let iseg = self.intersect(s);
        let lseg = self.split_at(s.start)?;
        let rseg = self.split_at(s.end)?;

        let (ls,is,rs) = (lseg, iseg, rseg);

What are you trying to do here?

        Ok((ls.0,is,rs.1))
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.