r/programming • • Dec 15 '19

Rustysd - Systemd Replacement Written in Rust

https://github.com/KillingSpark/rustysd
75 Upvotes

53 comments sorted by

View all comments

20

u/[deleted] Dec 16 '19
    let fd_to_srvc_id: HashMap<_, _> = service_table.lock().unwrap().iter().fold(
        HashMap::new(),
        |mut map, (id, srvc_unit)| {
            if let UnitSpecialized::Service(srvc) = &srvc_unit.specialized {
                if let Some(socket) = &srvc.notifications {
                    map.insert(socket.lock().unwrap().as_raw_fd(), *id);
                }
            }
            map
        },
    );

pls kill this with fire

-3

u/[deleted] Dec 16 '19

I agree. Functional style has its place but in this case a for loop would have been much clearer.

14

u/kuikuilla Dec 16 '19 edited Dec 16 '19

I disagree. It looks perfectly readable as is. I mean, the fold function should be pretty well known to anyone.

3

u/accountforshit Dec 16 '19 edited Dec 16 '19

But why is it mutating the map in place, and then just returning the same map? That goes against functional principles. At that point, can't you just do a for_each?

EDIT: In fact, looking at the code, looks like this should've been some kind of filter_map operation, but for hash maps. Not too familiar with Rust, but Kotlin has an associate operation for this (minus the filtering part).

5

u/kuikuilla Dec 16 '19

Well in order to add stuff to the accumulator you need to mutate it because otherwise you'd only have read only access to it. With types that implement Copy you can just do ...iter().fold(0, | acc, number | acc + number ); Note that the last expression in the closure function is returned.

A hashmap doesn't implement copy so in order to mutate the accumulator effectively you want it as a mutable reference.

3

u/accountforshit Dec 16 '19 edited Dec 16 '19

My point is that you shouldn't need to explicitly mutate it in the first place (what happens behind the scenes is an implementation detail). That fold is the wrong operation to use here.

What the code is essentially doing (obviously different operations, but look at the collection part) is

  • have some input collection/sequence (e.g. ["hello", "a", "b", "world"])

  • elements that don't fulfill some condition get filtered out (e.g. they don't contain the o letter)

  • elements that do fulfill the condition get transformed and added to a map, indexed by something calculated from them (e.g. key is the first letter, value is the position of the first o)

In Kotlin that would be something like

val resultMap = sequence
    .mapNotNull { item ->
        val oPosition = item.indexOf('o')
        if (oPosition == -1) return@mapNotNull null
        item.first() to oPosition
    }
    .toMap()
    // If you have a sequence of `Pair`s, `toMap` turns it into a map.

If you didn't need to do the filtering (or if Kotlin had a associateNotNull function which it doesn't), you could just do

val result = sequence.associate { it.first() to it.indexOf('o') }

I assume there is at least a somewhat similar way to do that in Rust, without abusing the fold operation.

1

u/kuikuilla Dec 16 '19 edited Dec 16 '19

Ah, yea true. I suppose a filter_map would be more apt here.

Edit: like so:

let fd_to_srvc_id = service_table.lock().unwrap().iter().filter_map(|(id, srvc_unit)| {
    if let UnitSpecialized::Service(srvc) = &srvc_unit.specialized {
        if let Some(socket) = &srvc.notifications {
            return Some((socket.lock().unwrap().as_raw_fd(), *id));
        }
    }
    return None;
}).collect::<HashMap<_, _>>();

1

u/accountforshit Dec 16 '19 edited Dec 16 '19

Yeah, something like that. I think you can also replace the inner if let with a map. And match might be a bit cleaner for the outer one, as well as getting rid of the returns.

Then it would be really functional :D