How to simplify this code?

[minimum]

How can I simplify this Rust code? Currently it's a bit hard to read due to complex nested blocks (11 levels) and many type conversion and unwraps. If possible, I want to return a Iterator.

The Python equivalent is here (43 LOC).

The logic is pretty simple: The code tries to parse the reading list file of Safari, yield all items and simply ignore errors like missing fields. (See also the comments on the Python script)

I read the doc of the plist library, followed and tried and found using and_then API of Option in all cases even makes the code much more hard to read. Also, I have no idea about how to remove the unwraps:

• Some of them maybe by return Result like the ones about path and reading file.
• Some of them like the get and as_* returns seems difficult to handle, since the goal is to return all possible items and simply ignoring issues like missing fields.

[H2CO3]

Typically, if you know the structure of a piece of serialized data, you should not be traversing it manually. Instead, use Serde to deserialize it to a statically-typed object, and traverse that object.

Also, all the unwrap()s make your code considerably more verbose and less robust to failure. Propagate (using the postfix ? operator) or handle the errors.

Here's an implementation of the above idea:

use std::io;
use std::error::Error;
use std::path::Path;
use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct Bookmarks {
    #[serde(rename = "Children")]
    children: Vec<Child>,
}

#[derive(Debug, Deserialize)]
struct Child {
    #[serde(default, rename = "Title")]
    title: String,
    #[serde(default, rename = "Children")]
    children: Vec<GrandChild>,
}

#[derive(Debug, Deserialize)]
struct GrandChild {
    #[serde(default, rename = "URLString")]
    url_string: String,
    #[serde(default, rename = "URIDictionary")]
    uri_dict: UriDictionary,
}

#[derive(Default, Debug, Deserialize)]
struct UriDictionary {
    title: String,
}

#[derive(Debug)]
struct Item {
    pub url: String,
    pub title: String,
}

fn get_readings<P: AsRef<Path>>(path: P) -> Result<impl Iterator<Item = Item>, Box<dyn Error>> {
    let bookmarks: Bookmarks = plist::from_file(path)?;

    let result = bookmarks
        .children
        .into_iter()
        .filter_map(|c| if c.title == "com.apple.ReadingList" {
            Some(c)
        } else {
            None
        })
        .flat_map(|c| c.children.into_iter().filter_map(|gc| {
            if !gc.url_string.is_empty() && !gc.uri_dict.title.is_empty() {
                Some(Item {
                    url: gc.url_string,
                    title: gc.uri_dict.title,
                })
            } else {
                None
            }
        }));

    Ok(result)
}

fn main() -> Result<(), Box<dyn Error>> {
    let path = <str as AsRef<Path>>::as_ref(env!("HOME"))
            .join("Library")
            .join("Safari")
            .join("Bookmarks.plist");
    // let path = "Bookmarks.plist";
    for item in get_readings(&path)? {
        dbg!(item);
    }

    Ok(())
}

[jofas]

@minimum

simply ignore errors like missing fields.

It does not ignore errors. You call unwrap a bunch of times which will cause your program to panic.

If you were open to return Option<Vec<String>> you could use the ? operator to make your code much more readable.

[minimum]

@jofas

If you were open to return Option<Vec<String>> you could use the ? operator to make your code much more readable.

In this case, isn't any None value from the Option will cause the function return None? But I want it just ignore the "issued" item. Like the check in the Python version:

if url and title:
    yield ...

[minimum]

Thanks for the suggestion. I gonna give a try. I didn't look deeply into the plist file, but maybe knowing the required filed is enough to use serde.

[H2CO3]

Check out my edited answer above.

[minimum]

Thanks a lot. I just come out my version. I gonna read and compare both versions.

[H2CO3]

Your version doesn't substantially simplify anything (still contains some 7 levels of nesting), and it doesn't handle missing fields correctly.

[Cocalus]

Rust Playground

Here's another edit with some cleanup's uses "let else" to ignore some errors in the format. I don't know which should be ignored and which imply the file is probably wrong, so I assumed unwrap meant the later.

[minimum]

It's because some fields have to be Option after I check the plist file so it becomes more complicated...

Isn't the filter_map enough remove invalid values since I use if let to check if the field exists? while you version checks if the strings are empty.

[minimum]

Thanks for the idea. Please see my first reply to jofas.

[H2CO3]

I could have used Option<String> instead of String, but since empty URLs and empty titles aren't meaningful, I simply slapped a #[serde(default)] on the fields to simplify the handling of missing values (since you have to check the contents of one of the Titles anyway).

[minimum]

Yes, gonna use both checks