How to simplify this code?

help
1

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.
2

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(())
}
2 Likes
3

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.

4

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 ...
5

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.

6

Check out my edited answer above.

7

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

8

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

9

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.

10

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.

11

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

12

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).

13

Yes, gonna use both checks :stuck_out_tongue:

14

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.