A character shift function in Rust

Question

As you might probably know, a Caesar cipher involves shifting/unshifting and wrapping up characters when they overflow the first or last letter of the alphabet, it's useful to have a function to just handle character shifting. So I wrote a function that I intend to use in other places and was hoping to see if there are any edge cases or gotchas that I might be missing in my current implementation

fn char_shift(ch: char, shift: i8) -> char {
    if shift == 0 || shift > 25 || shift < -25 {
        return ch;
    }
    let mut res = ch;
    let ch_i8 = ch as i8;
    match shift {
        // Positive Shift
        shift @ 0..=i8::MAX => {
            match ch_i8 {
                65..=90 => {
                    if ch_i8.overflowing_add(shift).1
                        || ch_i8.overflowing_add(shift).0 > 90
                    {
                        res = (ch_i8 + (shift - 90 + 64)) as u8 as char;
                    } else {
                        res = (ch_i8 + shift) as u8 as char;
                    }
                }
                97..=122 => {
                    if ch_i8.overflowing_add(shift).1
                        || ch_i8.overflowing_add(shift).0 > 122
                    {
                        res = (ch_i8 + (shift - 122 + 96)) as u8 as char;
                    } else {
                        res = (ch_i8 + shift) as u8 as char;
                    }
                }
                _ => { /* return the same character that it receieved */ }
            }
        }
        // Negative Shift
        unshift @ i8::MIN..=-1 => {
            let unshift = -unshift;
            match ch_i8 {
                65..=90 => {
                    if ch_i8 - unshift < 65 {
                        let diff = 65 - (ch_i8 - unshift);
                        res = ((90 - diff) + 1) as u8 as char;
                    } else {
                        res = (ch_i8 - unshift) as u8 as char;
                    }
                }
                97..=122 => {
                    if ch_i8 - unshift < 97 {
                        let diff = 97 - (ch_i8 - unshift);
                        res = ((122 - diff) + 1) as u8 as char;
                    } else {
                        res = (ch_i8 - unshift) as u8 as char;
                    }
                }
                _ => { /* return the same character that it receieved */ }
            }
        }
    }
    res
}

Answer 1

Function design

The function itself is well named. The main problem is that it is specific to the ABC, while that is just one alphabet. I would make it generic and have a class accept an alphabet which then can be used anywhere. This makes it more valuable for reuse.

Similarly, it handles upper and lowercase, while that is not clearly communicated. This is fine for a specific Caesar assignment, but it makes the function less useful. Things like upper / lowercase conversion or handling of characters outside of the alphabet can be performed by helper functions.

Code example

As I find it hard to comment on every line, and as this is a solved issue, I would like you to compare it with my code sample instead. The code does still adhere to your design (except for the handling of invalid shifts, although I have allowed for 0 after an edit).

Things to note:

• The use of constants, so that no literals are used in the function itself.
• The use of build-in methods that make the code that much more readable.
• The use of explicit guard statements on top of the code.
• The conversion to an index 0..25 before doing the shift.
• The use of modular addition (rem_euclid for the modulo-operation) on the shift instead of mucking around with characters.
• The use of the same block for positive and negative shifts, using the modular addition trick above (also, see "Remarks" below).
• The little trick of always calling to_ascii_uppercase and then checking if the input was lowercase at the end.
• The use of variable names that reflect their usage, not their type (input instead of ch, for example)

But the main thing I would like you to look at is the readability. Just show this to any one of your friends and ask them which one they think is more readable.

const ASCII_A: i8 = 'A' as i8;
const ASCII_Z: i8 = 'Z' as i8;
const ABC_SIZE: i8 = ASCII_Z - ASCII_A + 1;

fn char_shift(input: char, shift: i8) -> char {
    // --- guard statements
    if !input.is_ascii_alphabetic() {
        return input;
    }

    if shift >= ABC_SIZE || shift <= -ABC_SIZE {
       panic!("invalid shift");
    }

    // --- let's focus on uppercase only
    let input_upper = input.to_ascii_uppercase();

    // --- char to index within alphabet
    let input_index = input_upper as i8 - ASCII_A;

    // --- shift
    let output_index = (input_index + shift).rem_euclid(ABC_SIZE);

    // --- index to char
    let output_upper = ((output_index + ASCII_A) as u8) as char;

    // --- return, possibly after converting to lowercase
    if input.is_ascii_lowercase() {
        output_upper.to_ascii_lowercase();
    }
    output_upper
}

Remarks

Never just return the value if another parameter is out of bounds. This requires some way of error handling. Just returning an unencoded or worse unencrypted value is a security hazard.

A negative shift is the same as a positive shift mod 26, the size of the ABC, so there is no need for a separate code block. For ASCII you should really use u8 instead of i8. All calculations can be easily performed using u8 if you first convert the shift to a value in the range 0..25.

Note that they way that the function is written makes testing it a lot simpler too. Without so many literals (3 in total, and those are for the constant definitions) there aren't really (m)any "edge cases" left.