How to refactor those 2k lines?

Over the last two years I've created this monster and now I'd like to invest some time to improve it's readability & coding style.

Things that seem obvious points that need improving:

  1. split it into multiple modules, it has grown too big for a single file.
  2. better state handling so we don't have to pass it around as much.

I'm a bit lost on how to get started and wanted to ask if anybody has any suggestions for me.
I'd also value any constructive feedback unrelated to the above two goals.

Thank you for your time!

2 Likes

Write tests.

1 Like

thanks for the reply @jonh - tests are great, but that's not what I had in mind for this project.

I'm actually quite happy with what I've done to it today.

When I find time to continue with it, I plan on moving over the filtering logic from the function send_if_required and the resending from resend_if_required into the data_sender module and centralize the last sent store in some state there so I can later reuse it for an http interface to present it's content, and also a instant-messenger notification system for configurable conditions based on that cache.

I assume they just meant that having tests before a refactor will make it much easier to verify that something wasn't broken, and there aren't any in that file. But it's up to you of course.

7 Likes

It's also worth writing tests to see if it's difficult.

If things are mutated internally, the API is awkward, there are lots of (unnecessary?) dependencies on other modules' internals... And so on, it's hard to write tests.

This can help show what could be improved by a refactor.

But mostly yes, tests boost confidence that no behaviour has been accidentally changed by the refactor.

5 Likes

Having had a read through, I've definitely seen worse :slight_smile: it's good to nip things in the bud.

I'd extract some blocks into separate functions (especially the one setting up handles, the name should be obvious).
And some struct/enum definitions could be moved to a new module.

I can't comment on the state handling yet, it needs more thought and understanding (from me)

Not sure how clear you are on your exact target here, given the way you've formulated it - is your main objective "readability & coding style" or "better state handling" with all related to it?

The former is, to a larger extent, simply about moving things around a little bit.

The second is more of an architectural, higher-level, design problem.

Readability-wise, use early returns - for starters.

this is much harder to read
if let Ok(manager) = Manager::new() {
    let mut good_bats: Vec<Battery> = Vec::new();
    if let Ok(batteries) = manager.batteries() {
        for battery in batteries.into_iter().flatten() {
            good_bats.push(battery);
        }
    }
}

compared to this
let Ok(manager) = Manager::new() else { return; }
let mut good_bats: Vec<Battery> = Vec::new();
let Ok(batteries) = manager.batteries() else { return; }
for battery in batteries.into_iter().flatten() {
    good_bats.push(battery);            
}

Stop mixing different calls to the same methods.

why?
if debug {
    log("valetudo_wifi_watcher - json didn't contain valid signal strength.");
} else {
    println!(
        "{} - valetudo_wifi_watcher - json didn't contain valid signal strength: {}",
        timestamp(),
        to_string_pretty(&value).unwrap()
    );
}

// when

fn log(msg: &str) {
    println!("{} - {}", timestamp(), msg);
}

fn log_error(msg: &str) {
    eprintln!("{} - {}", timestamp(), msg);
}


Avoid unnecessary scopes.

again, why?
#[tokio::main]
async fn main() -> io::Result<()> {
    let mut handles = vec![];
    {
        let args = Cli::parse();
        // ...
    }
    futures::future::join_all(handles).await;
    // ...
}


I don't have the time to go through the whole repo, but these few things alone do make me wonder how much thought you've put into the overall structure of it all beforehand. Are you clear on your first go-to approach to logging and error-handling? Separation of concerns? Locality of behavior?

Pick up Dave Farley's "Modern Software Engineering" and - perhaps even more relevant to the topic at hand - "A Philosophy of Software Design", by John Ousterhout. Until you've committed to memory the change amplification / cognitive load / unknown unknowns trifecta, most of the lines you'll be writing will be turning into "monsters" of its own, sooner or later.

2 Likes

thank you so much for your replies, especially the beautiful easy to implement suggestion from @SDX - I didn't know this let Ok(x) = z else { .. } syntax existed, loving it :slight_smile:

why#1: started to refactor this logging the day before yesterday and only applied it to the easy to replace instances where no additional formatting happened as of yet.

why#2: I thought I can tell the compiler with this scope that all the memory allocations from inside that scope can be freed and are not needed while waiting on all the future handles in the end. Though I guess maybe he knows that anyway?

Well I've largely winged it and just added feature by feature when I needed them, without thinking about the overall architecture too much. That's what I'm trying to change by my current refactoring effort.


About adding tests - I think it's difficult because the data sources are unpredictable. - Can anyone give me pointers on how to write tests for this kind of project in rust? Are there any good guides / documentations / crates / examples you can point me to?

1 Like