HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Using a custom comparison function for BTreeSet and a foreign type

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionforeigncomparisontypecustomforusingandbtreeset

Problem

As detailed in this SO question, I'm using custom logic in a BTreeSet to automatically sort a foreign data type as elements are added to the container. Note that the Data type is used from another module in the actual implementation, so I can't just implement Ord for it, necessitating this approach.

I'm concerned that the iterator pipeline I'm using in main() is too complex and that the various impls are just boilerplate for something that really boils down to a single line of actual desired logic.

Here's a Rust Playground link.

use std::collections::BTreeSet;
use std::convert::{From, Into};
use std::cmp::Ordering;
use std::ops::Deref;

// Here's the original struct...
#[derive(PartialEq, Eq, Clone, Copy, Debug)]
struct Data(usize);

impl From for Data {
    fn from(i: usize) -> Self {
        Data(i)
    }
}

// And here's the wrapper type we'll use to sort it.
#[derive(PartialEq, Eq, Debug)]
struct SortableData(Data);

impl Deref for SortableData {
    type Target = Data;
    fn deref(&'a self) -> &'a Data {
        &self.0
    }
}

impl From for SortableData {
    fn from(data: Data) -> Self {
        SortableData(data)
    }
}

impl PartialOrd for SortableData {
    fn partial_cmp(&self, other: &Self) -> Option {
        Some(self.cmp(other))
    }
}

impl Ord for SortableData {
    fn cmp(&self, other: &Self) -> Ordering {
        (self.0).0.cmp(&(other.0).0)
    }
}

fn main() {
    let source: [Data; 3] = [Data(9), Data(4), Data(2)];
    let sorted: BTreeSet = source.iter().cloned().map(|i| i.into()).collect();
    for d in sorted.iter() {
        println!("{:?}", **d);
    }
}

Solution

Your code looks basically how I would write it, very nice! I do have a few tiny nits.

-
I strongly dislike using Deref except when you have a smart pointer. I fear that using Deref to compose items is going to be the Rust equivalent of using inheritance for code reuse.

Using Deref also can be a bit surprising, as every method is now available. Let's say that your Data struct has some more methods:

impl Data {
    fn wow(&self) { println!("Wow") }
}


Now, your SortableData has those methods as well!

for d in sorted.iter() {
    d.wow();
}


I much prefer to be explicit about it; I would implement Borrow instead.

I don't believe this opinion to be 100% supported by the community, but I'm going to make my case where I can ^_^

-
When possible, I'd implement From / Into in both directions. This doesn't always make sense, but here it does.

-
There's no need to specify the type for source — type inference handles that just fine.

-
You could just map the into function directly: .map(Into::into).

-
Instead of calling iter in the for loop expression, you can just take a reference.

-
From and Into are both part of the prelude and thus don't need to be used again.

use std::borrow::Borrow;
use std::cmp::Ordering;
use std::collections::BTreeSet;

// Here's the original struct...
#[derive(PartialEq, Eq, Clone, Copy, Debug)]
struct Data(usize);

impl From for Data {
    fn from(i: usize) -> Self {
        Data(i)
    }
}

// And here's the wrapper type we'll use to sort it.
#[derive(PartialEq, Eq, Debug)]
struct SortableData(Data);

impl From for SortableData {
    fn from(data: Data) -> Self {
        SortableData(data)
    }
}

impl From for Data {
    fn from(data: SortableData) -> Self {
        data.0
    }
}

impl Borrow for SortableData {
    fn borrow(&self) -> &Data {
        &self.0
    }
}

impl PartialOrd for SortableData {
    fn partial_cmp(&self, other: &Self) -> Option {
        Some(self.cmp(other))
    }
}

impl Ord for SortableData {
    fn cmp(&self, other: &Self) -> Ordering {
        (self.0).0.cmp(&(other.0).0)
    }
}

fn main() {
    let source = [Data(9), Data(4), Data(2)];
    let sorted: BTreeSet = source.iter().cloned().map(Into::into).collect();
    for d in &sorted {
        let d: &Data = d.borrow();
        println!("{:?}", d);
    }
}

Code Snippets

impl Data {
    fn wow(&self) { println!("Wow") }
}
for d in sorted.iter() {
    d.wow();
}
use std::borrow::Borrow;
use std::cmp::Ordering;
use std::collections::BTreeSet;

// Here's the original struct...
#[derive(PartialEq, Eq, Clone, Copy, Debug)]
struct Data(usize);

impl From<usize> for Data {
    fn from(i: usize) -> Self {
        Data(i)
    }
}

// And here's the wrapper type we'll use to sort it.
#[derive(PartialEq, Eq, Debug)]
struct SortableData(Data);

impl From<Data> for SortableData {
    fn from(data: Data) -> Self {
        SortableData(data)
    }
}

impl From<SortableData> for Data {
    fn from(data: SortableData) -> Self {
        data.0
    }
}

impl Borrow<Data> for SortableData {
    fn borrow(&self) -> &Data {
        &self.0
    }
}

impl PartialOrd for SortableData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

impl Ord for SortableData {
    fn cmp(&self, other: &Self) -> Ordering {
        (self.0).0.cmp(&(other.0).0)
    }
}

fn main() {
    let source = [Data(9), Data(4), Data(2)];
    let sorted: BTreeSet<SortableData> = source.iter().cloned().map(Into::into).collect();
    for d in &sorted {
        let d: &Data = d.borrow();
        println!("{:?}", d);
    }
}

Context

StackExchange Code Review Q#113010, answer score: 4

Revisions (0)

No revisions yet.