#Simplifying control flow

4 messages · Page 1 of 1 (latest)

toxic robin
#

Here's a bit of code that returns a training from a library, or if it is not in the library it also tries to find it in the database and add it to the library first:

https://pastebin.com/bv2aSL4Q (didn't fit in post)

I believe I messed up the structure which makes the control flow not obvious.
I asked AI to fix it and it came up with this:

for training_id in training_iter {
    let training_id = training_id.map_err(|e| format!("Error pulling worker trainings: {}", e))?;

    // get training from library
    let training = match TRAINING_LIB.with_lock(|lib| lib.get(&training_id.to_string())) {
        // if training is already in library, return it
        Some(training) => training,
        // if training is not in library, pull from database and add to library
        None => {
            let training = Training::pull_new(&training_id.to_string(), &conn)
                .map_err(|e| format!("Error pulling worker trainings: {}", e))?;

            let added_training = TRAINING_LIB.with_lock(|lib| {
                match lib.add(&training_id.to_string(), training) {
                    Some(training) => Ok(training),
                    None => Err(format!("Error pulling worker trainings: training with id {} not found", training_id)),
                }
            }).map_err(|e| format!("Error adding training to library: {}", e))?;

            added_training
        }
    };
}

the AI's solution looks much better, but I'd like to know how to approach something like this in the future so that i don't come up with such mangled mess
as in "What was the error in my thinking that made it come out like that"

toxic robin
#

Hm, i rewrote it in an even more concise way because I realized that there's no need to check the Option that results from adding, because that is None only when the training was already in library, which it can't be because we only add it if it isn't there yet in the first place

    for training_id in training_iter {
        let training_id = training_id.map_err(|e| format!("Error pulling worker trainings: {}", e))?;

        // get training from library
        let training = match TRAINING_LIB.with_lock(|lib| lib.get(&training_id.to_string())) {
            // if training is already in library, return i
            Some(training) => training,
            // if training is not in library, pull from database and add to library
            None => {
                TRAINING_LIB.with_lock(|lib| {
                    // pull training from database
                    let training = Training::pull_new(&training_id.to_string(), &conn)
                    .map_err(|e| format!("Error pulling worker trainings: {}", e))?;
                    // add training to library
                    lib.add(&training_id.to_string(), training).unwrap() // can unwrap because we check if it exists right before
                })
            }
        };

        worker_trainings.push(training);
    }
#

Also it could do with a single lock perhaps

#
    for training_id in training_iter {
        let training_id = training_id.map_err(|e| format!("Error pulling worker trainings: {}", e))?;

        let training = TRAINING_LIB.with_lock(|lib| -> Result<Weak<Mutex<Training>>, StringedError> {
            match lib.get(&training_id.to_string()) {
                Some(training) => Ok(training),
                None => {
                    let training =
                        Training::pull_new(&training_id.to_string(), &conn).map_err(|e| format!("Error pulling worker trainings: {}", e))?;

                    Ok(lib.add(&training_id.to_string(), training).unwrap())
                }
            }
        })?;

        worker_trainings.push(training);
    }