New to Rust, would love a code review

Hey all! I'm pretty new to Rust and did some work on a small, self-contained programming exercise. I've noticed that Rust often pushes you in the direction of a different design, which is something I am enjoying. Somewhat counter to that idea, I took a problem I've implemented in other languages (e.g. Python) and wanted to see what the experience would be like to "port" it over to Rust, realizing that it's probably best to rethink it from scratch next time.

In any event, the following is a very naive implementation of a "property management system". The system supports adding and removing units from a building. When a unit is added to a building, it is assigned a number. The numbers start sequentially from 1 and units are always assigned the lowest available number. For example, if three units are created and then added to the building, they would automatically be assigned the numbers 1, 2, and 3 respectively. If the second unit was removed (the one that was assigned the number 2) and another, new unit was then created and added to the building, it would be assigned the number 2 since that's now the lowest available unit number (rather than 4).

I'd love a review and I'm very interested in feedback/suggestions/guidance around how I'm handling mutability and borrowing (no surprise that this was the most challenging aspect for me). I more or less got this to work through a back and forth with the compiler. I'm sure there are different or better ways of doing everything here.

The code can be found here: Rust Playground

Thanks in advance!

Unfortunately I don't have time right now to offer an in depth review, but I just want to point out a couple of issues that popped out on a cursory read:

  • why are monetary amounts signed
  • its not clear to me that the if on line 74 does anything - if we executed line 78 unconditionally, I think we'd get the same outcome
  • you likely need to provide a get_unit_mut, because as written, the only useful things you can do with a Unit require it to be mutable, and you can't get a mutable Unit out of even a mutable Building. You can't even hack it by removing and re-adding the unit, because removing a unit makes it vanish into the ether, it's not returned.
  • is it even intentional that your model allows for "disembodied" Units that don't belong to any Building? The Rust ideal is that objects should be valid as soon as they become exposed to calling code, i.e. in this case, already installed in a Building.
2 Likes

All great points, thank you.

why are monetary amounts signed

Good point, this should definitely be changed. Thanks! That said, monthly income should remain signed since a building can have negative profit, i.e. where expenses exceed income.

its not clear to me that the if on line 74 does anything - if we executed line 78 unconditionally, I think we'd get the same outcome

Doh, you're absolutely right. Can delete five lines of code righ there.

you likely need to provide a get_unit_mut, because as written, the only useful things you can do with a Unit require it to be mutable, and you can't get a mutable Unit out of even a mutable Building. You can't even hack it by removing and re-adding the unit, because removing a unit makes it vanish into the ether, it's not returned.

I actually did have a originally have a get_unit_mut defined as

 fn get_unit_mut(&mut self, unit_number: u32) -> Option<&mut Unit> {
        self.units.iter_mut().find(|unit| unit.number == unit_number)
    }

but, to be honest, I can't now remember why. In any non-Rust language, I just took at as a given that I could freely mutate a Unit no matter where or how I got a handle to one. I think this is what I need to spend more time thinking about.

  • is it even intentional that your model allows for "disembodied" Units that don't belong to any Building? The Rust ideal is that objects should be valid as soon as they become exposed to calling code, i.e. in this case, already installed in a Building.

Yes, agreed this doesn't really make sense. I was doing this as an exercise based on set instructions which, as given, is pretty contrived (because it allowed for a notion of a Unit severed from a Building, which, agreed, does not make much sense). Anyhow, this is a really good point and I think my next step will be to unshackle myself from the instructions and take another pass at it, this time designing it the "right way" from scratch.

Thanks very much for taking a look, really appreciate it.

EDIT Just to add that another element that is contrived in the instructions for this exercise is the notion of adding/removing a unit altogether. What would it even mean to add/remove a unit from a Building? I suppose it could mean constructing a new one in a Building...? But, as you say, even in that case, it begins its life as a Unit of/in a Building.

1 Like

I live in a building where this has actually happened. Suite 312 no longer exists; there are now two suites (312A, 312B) that occupy the same space physically but are distinct from it legally and practically.

It's likely there was no period of time where that space was "not a unit," but there was definitely a period of time where that space was unoccupiable.

More pragmatically, there has to be some way for callers to create instances of the Building type. Adding units to a half-completed Building record is certainly an option, though I might be tempted to move that behaviour to a private trait so that callers can't freely add more units after the fact. There are others; one alternative is to have a constructor that takes a collection of Units as an argument.

I might suggest starting from a few behavioural tests or a small CLI program, rather than designing a data structure in a vacuum. Having to do work with your structure is one of the fastest ways to find the limits of your approach.

4 Likes

Great idea, thanks for the feedback

Your program currently deals with only gross incomes, you don't ever do a calculation that could potentially go negative. (Although yes, you would in the future)

A lot of containers do have an equivalent of get_mut (or equivalent IndexMut) for that reason, but a snag you might encounter in this context is that having a mutable reference is enough to wholesale replace the underlying data, if you have another instance to swap in.

    let mut building1 = Building::default();
    let mut building2 = Building::default();
    let unit1 = Unit::new("1 Bedroom".to_string(), 500, 100);
    let unit2 = Unit::new("2 Bedroom".to_string(), 1000, 200);
    building1.add_unit(unit1);
    building2.add_unit(unit2);
    let unit1 = building1.get_unit_mut(1).unwrap();
    let unit2 = building2.get_unit_mut(1).unwrap();
    std::mem::swap(unit1, unit2);
    // Your tenants are now very confused and upset

Additionally, assuming you're likely to actually have logic depend on the details of the unit_type, I'd make it an enum or a struct type unto itself - right now Unit::new("fifteen giraffes".to_string(), -500, 0) is a valid call.

Depending on which way around you want the coupling, you could have a building construct one (maybe with some data governing the unallocated dimensions) or have the constructor of Unit take a building to install the newly constructed unit into rather than return it

Thanks for the additional thoughts! I had forgotten to include the get_monthly_profit function which would've just returned self.get_monthly_income() - self.get_monthly_expenses() which could be negative.

In your example, did you mean to have to have get_unit_mut(1) for both or did you mean to write this:

 let unit1 = building1.get_unit_mut(1).unwrap();
 let unit2 = building2.get_unit_mut(2).unwrap();

I'm fairly sure what I wrote is right re: the code you gave, but looking at it I wasn't sure if it was intentional that the unit numbers vary per-building rather than globally. If it is supposed to be per-building, then swapping rooms around is not merely confusing but breaks the building's tracking of which numbers are in use and the assumption that they're unique