Seeking advice on designing/ implementing/ and testing a struct

This is a large post, and I realize there are many topics. Out of respect of your own time, please don't reply to every one of my concerns. Any response is appreciated.


I recently stumbled on a comment on r/learnprogramming that really hit home for me:

Most software is built by a team, not an individual. If you work on a project with more than one software engineer, you'll spend a lot of time collaborating with them - reviewing code, discussing how to divide things up, analyzing bugs.

As someone who is self learning, having support from a team is something that I desire so much. Like the thought of having a team to talk to when struggling to solve a problem. Or having someone point out flaws in my code, which would allow me ask myself the right questions.

I have been reading/ studying Rust for a while, but really needed to begin writing more Rust! I find it difficult to know where to start projects, especially with limited capabilities.

I finally came up with something...

I stumbled on an idea where I needed a data structure that allows me to customly define a range with a known position.

I assume something like this already exists, but I thought it would be a good exercise (By the way, if you know of an existing crate that already achieves this, please share).

My goal for this project is to write idiomatic Rust. :crab:

Context:

This struct will be used inside a game I am building. I needed a way to represent a a custom defined range that knows its current location.

For instance, I could define an attribute called confidence:

let mut confidence = RangePointer {
    min: 0,
    max: 100,
    current: 50
}

As the player interacts with game, their confidence can increase or decrease depending on an event.

Overview:

Functions include:

pub fn new() -> Self {}
pub fn increase(mut self) -> Result<u16, RangeError> {}
pub fn increase_by(mut self, amount: u16) -> Result<u16, RangeError> {}
pub fn decrease(mut self) -> Result<u16, RangeError> {}
pub fn decrease_by(mut self, amount: u16) -> Result<u16, RangeError> {}
pub fn randomize(mut self) -> Result<u16, RangeError> {}
pub fn minimize(mut self) -> Result<u16, RangeError> {}
pub fn maximize(mut self) -> Result<u16, RangeError> {}

Snippet:

Below is a snippet of my code, the whole project can be seen here on github. I commented on the areas that might be a questionable design choice. If you have any advice, even if it is a simple one liner, please share your thoughts. Thank you

#[derive(Debug, Eq, PartialEq, Copy, Clone)]
pub struct RangePointer {
    // Thought about making the type generic, 
    // but wouldn't this require building a separate trait?
    pub min: u16, 
    pub max: u16,
    pub current: u16
}

impl RangePointer {

    // What are your thoughts on returning a `Result`? Are there downsides to this approach?
    pub fn new(min: u16, max: u16, current: u16) -> Self {  // -> Result<RangePointer, RangeError>
        // Assert min < max, throw relevant error message
        // Assert current >= min and <= max, throw relevant error message
        RangePointer{ min, max, current }
    }
    
    // What are your thoughts on `mut self`? Should I consider `&mut self`?
    pub fn increase(mut self) -> Result<u16, RangeError> {
        let value = match self.current.overflowing_add(1) {
            (_, true) => self.max,
            (value, false) => value,
        };
        // Is there a more concise way to implement this `if` expression?
        if value > self.max {
            self.current = self.max;
        } else {
            self.current = value;
        }

        Ok(self.current)
    }

// This is my first custom error that I have ever wrote, and I referenced The Book
// Can this be improved?
// Would it be better to use a boxed std::error::Error?
#[derive(Debug, Clone)]
pub struct RangeError {
    pub message: String
}

impl fmt::Display for RangeError {
    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
        write!(f, "{}", self.message)
    }
}

impl std::error::Error for RangeError {
    fn description(&self) -> &str {
        &self.message
    }
}

#[cfg(test)]
mod tests {

    #[test]
    pub fn test_increase() {
        let r = RangePointer{min: 1, max: 6, current: 1};
        let a = r.increase().unwrap();
        assert_eq!(a, 2);
    }

    #[test]
    pub fn test_increase_limit() {
        let r = RangePointer{min: 1, max: 6, current: 6};
        let a = r.increase().unwrap();
        assert_eq!(a, 6);
    }

    #[test]
    pub fn test_increase_overflow() {
        let r = RangePointer{min: 1, max: u16::MAX, current: u16::MAX};
        let a = r.increase().unwrap();
        assert_eq!(a, u16::MAX);
    }
}

P.S.

I am very interested in learning how to write Clean Hexagonal Architecture with Data Oriented Design, but have trained myself to think in OOP. With time, I would like to retrain myself. If you see anything inside my code that would be a step in the right direction, please let me know.

There are no downsides that I am aware of. You must return a Result<Self, RangeError> or Option<Self> if you want the function to be fallible, without panicking.

You can use checked_add instead of overflowing_add to better convey your intent.

Currently, the method increase doesn't modify the range (actually, it does, but that can't be observed outside of the method, because it is dropped at the end of the method). This is probably not what you want. You should either borrow it with &mut self or, if you want to support method chaining, return Ok(self).

This is equivalent to

self.current = value.min(self.max);

I'm only a Rust noob but it seems to me that:

  1. I'm not keen on the name "RangePointer". It is not any kind of pointer. Perhaps "BoundedValue" or some such. Naming things nicely is the hardest part of programming.

  2. I would remove the "pub" from the structure fields. That stops anyone messing with the values directly, they have to go through the methods. Encapsulation and all that.

  3. New does not check that the given max is greater than the min or that current is in between.

  4. I think if it were me I would have the methods return "self" rather than "current"

  5. I might want to rename "current" as "value". If we want to think of it as a bounded value.

Thank you Aloso. These are all great insights. Thanks for pointing out &mut self, I should have noticed this. checked_sub is perfect. Implementing min() made my module 20 lines shorter :slight_smile:

Thanks for the input ZiCog. I like the name suggestion as well as changing current to value.

Encapsulating the fields is a great idea. However, this did present one new challenge in my integration tests. Since the fields are no longer public, I can no longer test my constructor like this:

#[test]
pub fn test_new() {
    assert_eq!(
        BoundedValue{min: 1, max: 6, value: 1}, 
        BoundedValue::new(1, 6, 1)
    );
}
error[E0451]: field `min` of struct `range_pointer::BoundedValue` is private
 --> tests/integration_tests.rs:7:22
  |
7 |         BoundedValue{min: 1, max: 6, value: 1}, 
  |                      ^^^^^^ private field

Hmm...

I'm wondering why you have "range_pointer" in that error message when the name should have been changed to "BoundedValue"?

I guess you could add some getters to get around the private fields. But that is icky.

This is because my package is named range_pointer. I renamed the struct BoundedValue

Sorry, my sneaky trick above was gibberish, I have deleted it.

If you have a module "bounded_value" and a struct "BoundedValue" and then put your tests inside the module then the fields do not need to be public and you can test your constructor:

mod bounded_value {
    #[derive(Debug, Eq, PartialEq, Copy, Clone)]
    pub struct BoundedValue {
        min: u16, 
        max: u16,
        value: u16
    }
...
...
    #[test]
    pub fn test_new() {
        assert_eq!(BoundedValue::new(1, 6, 3).min, 1);
        assert_eq!(BoundedValue::new(1, 6, 3).max, 6);
        assert_eq!(BoundedValue::new(1, 6, 3).value, 3);
    }
}

I guess one would call such tests "unit tests" as they are fiddling around with the units internals.

Integration tests on the other hand are outside of the module, as we are testing integration of modules, so they cannot do this.

But, given you have sufficient unit tests in place do you need to do such low level testing in the integration tests?

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.