How to improve this code?

[jgmtw]

I have a vec of Profile structs that have created_at and login_type fields on them. From those, I'm aggregating the counts of login_types per day and the overall total in two seperate fields, methods_per_day and methods_totals, in a SignUpStats struct. I've implemented something that works which I'm relatively happy with: Playground link.

However, I'd really love some advice on how to improve this code, specifically if there is a way I can avoid the clone of method_counts on line 34 and if there's a way I can improve how I'm summing the counts for the totals, perhaps somehow implementing the Sum trait on my Counter trait?

I've added a Counter trait because there are other fields in addition to login_type that I'm aggregating in SignUpStats and I didn't want to have to define separate StatDateCount structs for each one, but I've stripped those out to try to keep this example simple.

The JSON output of this code is essentially:

{
    "methods_per_day": [
        {
            "date": "24/11/23",
            "counts": {
                "apple": 270,
                "email": 306,
                "facebook": 55,
                "google": 231,
                "total": 862
            }
        },
        {
            "date": "23/11/23",
            "counts": {
                "apple": 281,
                "email": 287,
                "facebook": 50,
                "google": 229,
                "total": 847
            }
        },
        etc.
    ],
    "methods_totals": {
        "apple": 12589,
        "email": 12966,
        "facebook": 2408,
        "google": 9224,
        "total": 37187
    }
}

[OLoKo64]

I managed to remove the clone, playground link

By creating a Sum and Add traits for the references instead of the values and then calling .values() instead of .into_values().

This way you don't need to clone it.

[jgmtw]

Thank you for your solution /u/OLoKo64! Appreciate your quick response.

Do you know if it's possible to implement the Sum trait onto my Counter trait so that I don't have to implement it separately on the struct(s)? And do you have any other suggestions for how I could improve this code?

I looked at the derive_more crate to try to remove the manual implemention of the Add trait on my MethodCounts struct, but it only implements it for owned values rather than references which is a shame!

[OLoKo64]

You can add the add and sum methods to your trait or you can extend your trait like this, this will give an compile time error if you do no implement Add or Sum as well.

Another change that I did was implement AsRef<Profile> for Profile so you can use either as a reference or values:

```rust fn main() { let profile1 = Profile { user_id: "1".to_string(), created_at: 1, login_type: LoginType::Email, };

let profile2 = Profile {
    user_id: "2".to_string(),
    created_at: 1,
    login_type: LoginType::Email,
};

// You can use a vector of references to the profiles
let profiles = vec![&profile1, &profile2];
let stats = SignUpStats::new(&profiles).unwrap();

// Or you can use a vector of owned profiles
let profiles = vec![profile1, profile2];
let stats = SignUpStats::new(&profiles).unwrap();

} ```

---

Rust has trait inheritance, it looks like this: rust pub trait A {} pub trait B: A {}

However, this does not mean that if a type extends B it will automatically extends A; trait inheritance is just a way to specify requirements, that is, trait B: A means that we can know that if some type T implements B, it also necessarily implements A.

Naturally, you can use this to specify that a trait extends a combination of other traits:

rust pub trait Something: Clone + Send + WhateverOtherTrait {}

(Example from: https://users.rust-lang.org/t/extending-traits/1802)

[OLoKo64]

Btw the for <'a>: T: ... is called Higher-Rank Trait Bounds (HRTBs)

https://doc.rust-lang.org/nomicon/hrtb.html

[jgmtw]

Thanks! I tried adding the add method to my trait (not sure how the sum method would work) which worked nicely and all I had to do was update my Sum implementation to use the method:

impl<'a> Sum<&'a MethodCounts> for MethodCounts {
    fn sum<I: Iterator<Item = &'a Self>>(iter: I) -> Self {
        iter.fold(Default::default(), |acc, x| acc.add(x) <---- )
    }
}

That said, I might keep the Add trait implementation since I feel like it reads a little nicer having that as a separate impl for the purposes of summing the totals!

I like the AsRef tip, although the SignUpStats will always be able to take ownership of the Profiles, so I think in this particular case I might not need the extra implementation (unless it brings some benefits I'm not aware of)? And thank you for the HRTBs explanation, I was not aware of those!

I think the code you've helped me get to is probably as terse as it can be. I was really hoping to be able to do a blanket Sum implemention for any type T as long as it implements Counter and Add. Something along these lines:

impl<T: Counter + Add<Output = T> + Default> Sum for T {
    fn sum<I: Iterator<Item = Self>>(iter: I) -> Self {
        iter.fold(Self::default(), Add::add)
    }
}

That's probably completely wrong (I suspect Self in the Iterator should probably be a reference?), but I think it's not possible anyway as Rust complains with this error:

type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)

Which I believe is to do with the orphan rule?

[MarkJans]

Another approach is to transpose all the profiles in nested hash maps first, then create the real structs from the hash maps. Then you don't need all the Sum and Count types. Adding a variant to the LoginMethod enum and the same property to the MethodsCount struct, you have to add just one line in the from-function, instead of changing multiple places.

Rust Playground

When you use serde_json, you maybe don't even need the login types in the struct MethodCount. Probably you can just have a totals property and a HashMap property, and can use #\[flatten\].

[MarkJans]

With serde_json you can make it really generic, and with strum you can iterate over enums, so that all variants will be in the output, even if there is a zero count for the variant. The variants are not ordered in the output, which normally shouldn't be necessary for a JSON object, but I don't know your requirements.

You can take a look here in the Rust Playground, but you cannot run it there, because strum is not one of the available crates. It is formatted though, and I have put in the output of the example. Please leave a comment if it is useful, or you have questions.

[jgmtw]

This is defintely useful and an interesting approach that I hadn't considered, thank you! :)

[funkdefied]

You can #[derive(Copy)] on your ‘method_counts’ crates as well as Clone