How to improve this getting method

help
1

I'm implementing an object that is supposed to cache JSON resources. You give it a filename, if it's new: it loads the file, parses it, inserts it into the cache, and returns you a reference to the parsed result. If the filename is in the cache already, it just returns a reference.

This is the method that achieves that:

// self.objects: HashMap<ContentReference, ObjectDescription>

pub fn get_object<'a>(
    &'a mut self,
    cref: &ContentReference,
) -> Result<&'a ObjectDescription, CacheError> {
    if self.objects.contains_key(cref) {
        Ok(self.objects.get(cref).unwrap())
    } else {
        let file = fs::read_to_string(content::path_from_ref(cref))?;
        let desc = serde_json::from_str(&file)?;
        self.objects.insert(cref.to_owned(), desc);
        Ok(self.objects.get(cref).unwrap())
    }
}

I'm not exactly pleased with these unwraps and I'm wondering whether there's a better approach that would use match for example.

This was my initial attempt:

pub fn get_object<'a>(
    &'a mut self,
    cref: &ContentReference,
) -> Result<&'a ObjectDescription, CacheError> {
    match self.objects.get(cref) {
        None => {
            let file = fs::read_to_string(content::path_from_ref(cref))?;
            let desc = serde_json::from_str(&file)?;
            self.objects.insert(cref.to_owned(), desc);
            Ok(self.objects.get(cref).unwrap())
        }
        Some(desc) => Ok(desc),
    }
}

It still uses an unwrap though, but I think it's better in spirit. However, it's rejected because self.objects is borrowed mutably by insert.

2

Can you please share some more background code? E.g. what is self.objects?

It would be good, if you can provide a simple example on the playground and share it with us here so we can help you better. In the upper right corner is a share button which can be used to get a link to the code your inserted there.

3

If objects is a HashMap (or similar), there is the entry function which can be used for the Entry pattern which often is very handy to use.

1 Like
4

God damn it I'm bad at multitasking. Here's the playground link.

I know about Entry but I'm not sure if it can be used here, due to the need to return errors and such.

5

An interesting post to read:

FIY: There is polonius which solves some problems of the current borrow checker, but it's only available on nightly.

It's okay what you are doing I think, there not much you can improve unfortunately.

6

Okay, thank you for the information! I'll stick to the current solution then. I'm making an early prototype anyway, and as a Rust newbie I was more interested in whether I was maybe missing something important about the language.

7

Using the entry api might be possible but I could not get it working because Rust does not allow returning (?) from a closure:

    pub fn get_object<'a>(
        &'a mut self,
        cref: &ContentReference,
    ) -> Result<&'a ObjectDescription, CacheError> {
        Ok(self.objects.entry(cref.to_owned()).or_insert_with(|| {
            let file = fs::read_to_string(cref)?;
            serde_json::from_str(&file)?
        }))
    }

makes a comile error.

But you can get rid of the unwraps:

    pub fn get_object<'a>(
        &'a mut self,
        cref: &ContentReference,
    ) -> Result<&'a ObjectDescription, CacheError> {
        if let Some(desc) = self.objects.get(cref) {
            Ok(desc)
        } else {
            let file = fs::read_to_string(content::path_from_ref(cref))?;
            let desc = serde_json::from_str(&file)?;
            self.objects.insert(cref.to_owned(), desc);
            Ok(desc)
        }
    }
8

In this case you can match on the entry and manually reproduce the behavior of or_insert_with.

use std::collections::hash_map::Entry;

pub fn get_object<'a>(
    &'a mut self,
    cref: &ContentReference,
) -> Result<&'a ObjectDescription, CacheError> {
    match self.objects.entry(cref.to_owned()) {
        Entry::Occupied(entry) => Ok(entry.get()),
        Entry::Vacant(entry) => {
            let file = fs::read_to_string(cref)?;
            let value = serde_json::from_str(&file)?;
            Ok(entry.insert(value))
        },
    }
}

Unfortunately this approach clones cref even if it already exists.

9

You may also run into problems later, if you want to call the method more than once before dropping the old reference it returned. That's because your method takes &mut self, instead of &self. If that happens, you should take a look at std::cell::RefCell.

10

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.