Get rid of duplicate code

Hello

I have a login-function in my desktop-app. When the user logs in I save the session-id in a save/encrypted file using Cocoon. I have made a module called session.rs with two functions: get_session() and set_session(id: String). The first functions opens the Cocoon-file and retrieves the session-id if the Cocoon-files exist. The other functions opens the Cocoon file and inserts the given id.

pub fn get_session() -> Option<String> {
    let mut session_storage_file = std::fs::OpenOptions::new()
        .read(true)
        .write(true)
        .append(false)
        .create(true)
        .open("session.txt")
        .unwrap();

    let cocoon = cocoon::Cocoon::new(&*config::COCOON_KEY.as_bytes());
    match cocoon.parse(&mut session_storage_file) {
        Ok(r) => Some(String::from_utf8(r).unwrap()),
        Err(_) => None,
    } 
}

pub fn set_session(session_id: String) {
    let mut session_storage_file = std::fs::OpenOptions::new()
        .read(true)
        .write(true)
        .append(false)
        .create(true)
        .open("session.txt")
        .unwrap();

    let cocoon = cocoon::Cocoon::new(&*config::COCOON_KEY.as_bytes());
    cocoon.dump(session_id.as_bytes().to_vec(), &mut session_storage_file).unwrap(); 
}

The code works but because I need to open the file and make a cocoon-object each time I'll have the following duplicate code in all my functions in this module:

 let mut session_storage_file = std::fs::OpenOptions::new()
        .read(true)
        .write(true)
        .append(false)
        .create(true)
        .open("session.txt")
        .unwrap();

    let cocoon = cocoon::Cocoon::new(&*config::COCOON_KEY.as_bytes());

Is there a way to get rid of this duplicate code? Maybe make some kind of struct like struct Session {file: std::fs::File}? How would I do this?

Currently solved it like this:

// Reusable function
fn cocoon<'a>() -> cocoon::Cocoon<'a, cocoon::Creation> {
    cocoon::Cocoon::new(&*config::COCOON_KEY.as_bytes())
}
// Reusable function
fn session_storage_file() -> std::fs::File {
    std::fs::OpenOptions::new()
        .read(true)
        .write(true)
        .append(false)
        .create(true)
        .open("session.txt")
        .unwrap()
}

pub fn get_session() -> Option<String> {
    match cocoon().parse(&mut session_storage_file()) {
        Ok(r) => Some(String::from_utf8(r).unwrap()),
        Err(_) => None,
    } 
}

pub fn set_session(session_id: String) {
    cocoon().dump(session_id.as_bytes().to_vec(), &mut session_storage_file()).unwrap(); 
}

Is this the recommended way?

Short version of the most important part from below: Don't abstract over opening-for-reading and opening-for-writing. Use File::open for the former and File::create for the latter.

Second most important is to build error handling into your code now instead of needing to tack it on later when your code panics due to an unwrap or whatever. Even if you just bubble it to some other panic spot in your code, more precise errors can save you time later.


get_session, set_session and session_storage_file should return a Result.

Futher below I do a sort of "first pass" of error handling. Even better error handling is possible, but takes more work.

fn cocoon<'a>() -> cocoon::Cocoon<'a, cocoon::Creation> {
    cocoon::Cocoon::new(&*config::COCOON_KEY.as_bytes())
}

Just make that return a Cocoon<'static, _> and ditch the lifetime parameter on the function.

    std::fs::OpenOptions::new()
        .read(true)
        .write(true)
        .append(false)

All OpenOptions are false by default so you don't need the append call.

If you don't need both read and write, I wouldn't open it with both, nor create it if only reading. That is, open/create files with the abilities you need, and without the abilities you don't need. Looking at the cocoon create, you only need Read to parse and Write to dump.

So for reading, you can just use File::open. For writing, you can just use File::create.

You're currently overwriting the file contents without truncating the file (which could cause problems if the length is not fixed). But you don't want to truncate when reading the file of course, illustrating a concrete reason not to try and abstract over why you're opening the file.

    match cocoon.parse(&mut session_storage_file) {
        Ok(r) => Some(String::from_utf8(r).unwrap()),
        Err(_) => None,
    } 

Without considering the other suggestions, the match could be rewritten as

cocoon
    .parse(&mut session_storage_file)
    .map(|r| String::from_utf8(r).unwrap())
    .ok()

If nothing else, replace unwrap with .expect("Non-UTF8 in decrypted session key") or something.


Taking a step back, maybe you could have something like...

/// N.b. I wrote this on the forum only so there may be
/// some typos or other errors

#[derive(Clone, Debug)]
pub(crate) struct Session(String);

// Discussion over hard-coding this and not having a
// directory path can wait for another day
static SESSION_FILE: &Path = Path::new("session.txt");

impl Session {
    fn cocoon() -> cocoon::<Cocoon<'static, cocoon::Creation> {
        cocoon::Cocoon::new(&*config::COCOON_KEY.as_bytes())
    }

    pub fn from_session_file() -> Result<Option<Self>, SessionError> {
        let mut file = match fs::open(SESSION_FILE)
            Ok(f) => f,
            Err(e) if e.kind() == ErrorKind::NotFound => return Ok(None),
            Err(e) => {
                let something_descriptive = todo!();
                return Err(something_descriptive);
            }
        })?;

        let r = Self::cocoon()
            .parse(&mut file)
            .map_err(SessionError::Parse)?;

        let session = String::from_utf8(r).map_err(|_e| {
            // ...maybe store the source error too...
            SessionError::InvalidSession
        })?;

        Ok(Some(Self(session)))
    }

    pub fn store_session_file(self) -> Result<(), SessionError> {
        let mut file = fs::create(SESSION_FILE).map_err(|e| {
            // ...some descriptive SessionError::
        })?;

        Self::cocoon()
            .dump(self.0.into_bytes(), &mut file)
            .map_err(SessionError::Dump)
    }
}

pub(crate) enum SessionError {
    InvalidSession,
    Parse(cocoon::Error),
    Dump(cocoon::Error),
    // ...
}

This illustrates another concrete reason to not abstract over opening files: you want a missing file to indicate no session, but to check that properly, you need to try to open it without potentially creating it. (And if there's a permission problem or whatever, that should still be an error.)

You're slightly hampered by cocoon's design decisions (taking Vec<u8> by value, taking &mut impl Write and &mut impl Read). You might be able to work around them with the lower-level methods and more code on your end; I didn't attempt such. You could make store_session_file take &self if you don't mind always cloning the String.

Some of the map_errs could be turned into just ? with the right impl From<_> for SessionError additions, but the quality of error reporting may be worse if you do so.

4 Likes

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.