Skip to content

Conversation

@vgasparyan1995
Copy link
Contributor

Hi @wpwoodjr ,

I tried to implement a version of the generic stalin sort that takes in and returns an iterator. It looks ugly, but works as intended. I tried to get rid of the Clone requirement, but wasn't creative enough.

This PR is rather an ask of a review from someone who has looked into this problem. I'd appreciate it if you could review this code and suggest some improvements. I believe it has a lot of room for it.

Thanks in advance,
Vanand

@wpwoodjr
Copy link
Contributor

wpwoodjr commented Oct 15, 2024

Hi @vgasparyan1995

There's a lot going on in your code, I didn't really understand it to be honest, but I'm guessing it is not O(n).
Give this a try. It is O(n) and allocates a new vec of equal or less size to the old one, with no clones.

// Author Bill Wood
fn main() {
    let x = vec!(1,2,3,2,5,8,4,9);
    println!("{:?}", stalin_sort(x));
    let x = vec!('a','b','c','b','e','g','d','x');
    println!("{:?}", stalin_sort(x));
    let ideologies = vec!(
        "Fully Automated Luxury Space Communism",
        "Socialism",
        "Capitalism",
        "Communism",
    );
    println!("{:?}", stalin_sort(ideologies));
}

fn stalin_sort<T: PartialOrd + Clone>(x: Vec<T>) -> Vec<T> {
    stalin_sort_iter(x.into_iter()).collect()
}

fn stalin_sort_iter<T: PartialOrd>(
    input: impl Iterator<Item = T>,
) -> impl Iterator<Item = T> {
    let mut output = vec![];
    for v in input {
        if output.len() == 0 || &v >= output.last().unwrap() {
            output.push(v);
        }
    }
    output.into_iter()
}

@vgasparyan1995
Copy link
Contributor Author

Hi @wpwoodjr ,

Thanks for the quick review and response. Sorry for my late response.

My code aims to make the function more generic by lifting the Vec requirement. stalin_sort(Vec<T>) -> Vec<T> requires the input items to be in a Vec, to be in a contiguous memory, where they are random-accessible. If I may put it in my native C++, it's like having void stalin_sort(RandomAccessIt begin, RandomAccessIt end) when ForwardIt is a looser and more appropriate requirement. Some things, that are naturally "stalin-sortable", like streams and infinite iterators, can't be stalin-sorted with the existing signature.

As for the time complexity of my solution, I agree, it's not O(N), it's rather O(1). stalin_sort_iter simply wraps the input iterator in a Scan and FilterMap, it doesn't consume the input. But I assume you're not talking about the stalin_sort_iter specifically, but rather the the code that consumes it. If Rust's zero cost abstraction claims are true, then this approach doesn't change the asymptotic time-complexity. Here's what it effectively "turns" into:

for item in arr {
  let scan_state = ...;
  let scan_result = scan_operation(scan_state, item);
  if (filter_operation(scan_result)) {
    output.push(map_operation(scan_result));
  }
}

As for the clones, you might have noticed that I added the Clone requirement on the item type, which seemingly conflicts with the aim of making it more generic. In your Vec implementation you don't need that because you collect the result in a Vec and have a reference to the last item there. With my lazy iter approach I couldn't find a way around it. I guess, in a way, it's a trade-off.

@vgasparyan1995
Copy link
Contributor Author

@gustavo-depaula , please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants