How to improve this getting method

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.

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.

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

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.

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.

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.

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

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.

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.