(Ab)using Iterator?

code review
1

Am I using or abusing iterators here? Can this be considered good coding. Why or why not?

I'm fairly new to rust. Any feedback is very much appreciated :slight_smile:

use chrono::prelude::{DateTime, Utc};
use core::time;
use std::thread;

#[derive(Debug)]
struct Search {
    current_result: SearchResult,
}

impl Search {
    pub fn new() -> Self {
        Self {
            current_result: SearchResult { result: 0 },
        }
    }

    pub fn iter(&mut self) -> SearchIterator {
        SearchIterator { search: self }
    }

    fn step(&mut self) {
        // doing some calculations here
        thread::sleep(time::Duration::from_millis(1500));
        self.current_result = SearchResult {
            // store this iterations' result
            result: self.current_result.result + 1,
        }
    }
}

struct SearchIterator<'a> {
    search: &'a mut Search,
}

impl<'a> Iterator for SearchIterator<'a> {
    type Item = SearchIterationResult;

    fn next(&mut self) -> Option<Self::Item> {
        self.search.step();
        Some(SearchIterationResult {
            finished: Utc::now(),
            current_result: self.search.current_result.clone(),
        })
    }
}

#[derive(Debug, Clone)]
struct SearchResult {
    result: isize,
}

#[derive(Debug)]
struct SearchIterationResult {
    finished: DateTime<Utc>,
    current_result: SearchResult,
    // plus some more metrics on the iteration
}

fn main() {
    let mut search = Search::new();

    for result in search.iter().take(5) {
        let dt: DateTime<Utc> = result.finished.into();
        println!(
            "Finished interation at {} with result {}",
            dt.to_string(),
            result.current_result.result
        )
    }

    println!(
        "Search finished with result {}",
        search.current_result.result
    )
}
2

Which part of this do you think is problematic? The code seems fine to me, except that iter() should probably be called iter_mut().

2 Likes
3

On first look, this looks relatively sane. Feel free to elaborate what aspects of this code you feel have the potential of being considered “abuse” :wink:

Also, running cargo clippy will correctly point out a useless call to .into() (unnecessarily converting to the same type it already was) as well as a useless call to .to_string() (unnecessarily turning a type that implements Display into a String before passing result to println!) in this code, but that’s unrelated to the main question.

2 Likes
4

Just a small nit, but I would probably try get rid of SearchIterationResult by putting the finished field into SearchResult itself and return that from next. I don't know your context and this may not be feasible/desired for your real problem, so feel free to ignore this

5

Thank you both for your quick replies @H2CO3 @steffahn. I wasn't sure if a mutable reference to the "collection" is considered acceptable if I don't intend to obtain a mutable reference to the collection's items like with an iter_mut on a Vec.

@steffahn Thanks for pointing out clippy. Haven't come across that.

6

What I'd like to achieve basically is, to collect some metrics during the iteration (and print them to stdout or similar) and access the final search result after the iteration from Search. Does that make sense?

7

Yes, that makes total sense. Will you be calling the step method anywhere else or only in SearchIterator::next?

8

Only in the SearchIterator::next.

9

Then I'd probably change your interface a little so I'd have all the functionality (result + meta information gathering) in a single place, namely step. Or if you have a lot of metadata, a dedicated private method on Search rather than doing it directly in Iterator::next. I find this design to be conceptually easier to follow than having to look across implementation blocks, but again, this is opinionated and only a nitpick:

    ...

    fn step(&mut self) -> SearchIterationResult {
        // doing some calculations here
        thread::sleep(time::Duration::from_millis(1500));
        self.current_result = SearchResult {
            // store this iterations' result
            result: self.current_result.result + 1,
        };

        SearchIterationResult {
            finished: Utc::now(),
            current_result: self.current_result.clone(),
        }
    }
}

struct SearchIterator<'a> {
    search: &'a mut Search,
}

impl<'a> Iterator for SearchIterator<'a> {
    type Item = SearchIterationResult;

    fn next(&mut self) -> Option<Self::Item> {
        Some(self.search.step())
    }
}
10

Why are Search and SearchIterator two separate types? The way I see it here it would probably make sense to just make it one struct. You kind of have the iteration state spread between two instances.

1 Like
11

That's good advice. I like it.

12

That was my initial thought, too. But I couldn't get it work implementing Iterator for Search.

Could you point me into a direction?

13

Now with lifetimes...

use chrono::prelude::{DateTime, Utc};
use core::time;
use std::thread;

#[derive(Debug)]
struct SearchSpace {
    alpha: isize,
}

#[derive(Debug)]
struct Search<'a> {
    search_space: &'a SearchSpace,
    current_result: SearchResult,
}

impl<'a> Search<'a> {
    pub fn new(search_space: &'a SearchSpace) -> Self {
        Self {
            search_space,
            current_result: SearchResult { result: 0 },
        }
    }

    pub fn iter(&'a mut self) -> SearchIterator<'a> {
        SearchIterator { search: self }
    }

    fn step(&mut self) -> SearchIterationResult {
        thread::sleep(time::Duration::from_millis(1500));
        self.current_result = SearchResult {
            result: self.current_result.result + 1,
        };

        SearchIterationResult {
            finished: Utc::now(),
            current_result: self.current_result.clone(),
        }
    }
}

struct SearchIterator<'a> {
    search: &'a mut Search<'a>,
}

impl<'a> Iterator for SearchIterator<'a> {
    type Item = SearchIterationResult;

    fn next(&mut self) -> Option<Self::Item> {
        Some(self.search.step())
    }
}

#[derive(Debug, Clone)]
struct SearchResult {
    result: isize,
}

#[derive(Debug)]
struct SearchIterationResult {
    finished: DateTime<Utc>,
    current_result: SearchResult,
}

fn main() {
    let search_space = SearchSpace { alpha: 10 };

    let mut search = Search::new(&search_space);

    for result in search.iter().take(5) {
        let dt: DateTime<Utc> = result.finished.into();
        println!(
            "Finished interation at {} with result {}",
            dt.to_string(),
            result.current_result.result
        )
    }

    println!(
        "Search finished with result {}",
        search.current_result.result
    )
}

I get following error:

   |
69 |     for result in search.iter().take(5) {
   |                   ------------- mutable borrow occurs here
...
80 |         search.current_result.result
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |         |
   |         immutable borrow occurs here
   |         mutable borrow later used here
   |

I guess, I understand why.

    pub fn iter(&'a mut self) -> SearchIterator<'a> {
        SearchIterator { search: self }
    }

Here I say that the SearchIterator needs to outlive Search. But that isn't really necessary, is it? Can't find any other formulation though.

14

Solved my own problem :slight_smile:

I was on the right track to specify different lifetimes. Man, I love this explicity of Rust.

use chrono::prelude::{DateTime, Utc};
use core::time;
use std::thread;

#[derive(Debug)]
struct SearchSpace {
    alpha: isize,
}

#[derive(Debug)]
struct Search<'a> {
    search_space: &'a SearchSpace,
    current_result: SearchResult,
}

impl<'a, 'b> Search<'a> {
    pub fn new(search_space: &'a SearchSpace) -> Self {
        Self {
            search_space,
            current_result: SearchResult { result: 0 },
        }
    }

    pub fn iter(&'b mut self) -> SearchIterator<'a, 'b> {
        SearchIterator { search: self }
    }

    fn step(&mut self) -> SearchIterationResult {
        thread::sleep(time::Duration::from_millis(1500));
        self.current_result = SearchResult {
            result: self.current_result.result + 1,
        };

        SearchIterationResult {
            finished: Utc::now(),
            current_result: self.current_result.clone(),
        }
    }
}

struct SearchIterator<'a, 'b> {
    search: &'b mut Search<'a>,
}

impl Iterator for SearchIterator<'_, '_> {
    type Item = SearchIterationResult;

    fn next(&mut self) -> Option<Self::Item> {
        Some(self.search.step())
    }
}

#[derive(Debug, Clone)]
struct SearchResult {
    result: isize,
}

#[derive(Debug)]
struct SearchIterationResult {
    finished: DateTime<Utc>,
    current_result: SearchResult,
}

fn main() {
    let search_space = SearchSpace { alpha: 10 };

    let mut search = Search::new(&search_space);

    for result in search.iter().take(5) {
        let dt: DateTime<Utc> = result.finished.into();
        println!(
            "Finished interation at {} with result {}",
            dt.to_string(),
            result.current_result.result
        )
    }

    println!(
        "Search finished with result {}",
        search.current_result.result
    )
}
1 Like