Convert 9-digit integers to ISBNs

Question

Problem statement: The International Standard Book Number (ISBN) is a 10 digit code that uniquely specifies a book. The rightmost digit is a checksum digit which can be uniquely determined from the other 9 digits from the condition that \$d_1 + 2d_2 + 3d_3 + ... + 10d_{10}\$ must be a multiple of 11 (here \$d_i\$ denotes the \$\text{i}_{\text{th}}\$ digit from the right). The checksum digit \$d_1\$ can be any value from 0 to 10: the ISBN convention is to use the value X to denote 10. Write a program that takes a 9-digit integer digits as a command-line argument, computes the checksum, and prints the 10-digit ISBN number.

This is one of my self-imposed challenges in Rust to become better at it. The problem was taken from Sedgewick Exercise 1.3.35.

Here is my code:

use clap::Parser;

#[derive(Debug, Parser)]
struct Arguments {
    #[arg(index = 1)]
    digits: String,
}

fn main() {
    let arguments = Arguments::parse();
    let isbn = calculate_isbn(&arguments.digits);

    println!("{}", isbn);
}

fn calculate_isbn(digits_string: &String) -> String {
    let checksum = calculate_checksum(&digits_string);
    let mut isbn = digits_string.clone();

    match checksum {
        10 => isbn.push_str("X"),
        _ => isbn.push_str(&checksum.to_string()),
    }

    isbn
}

fn calculate_checksum(digits_string: &String) -> u32 {
    let digits_vector = convert_string_to_vector(digits_string);
    let checksum_vector = vec![10, 9, 8, 7, 6, 5, 4, 3, 2];
    let dot_product: u32 = digits_vector
        .iter()
        .zip(checksum_vector.iter())
        .map(|(x, y)| x * y)
        .sum();
    let mut checksum: u32 = 0;

    for index in 0..=10 {
        if (dot_product + index) % 11 == 0 {
            checksum = index as u32;
            break;
        }
    }

    checksum
}

fn convert_string_to_vector(digits_string: &String) -> Vec<u32> {
    let digits_vector = digits_string
        .chars()
        .map(|character| character.to_digit(10).unwrap())
        .collect();
    digits_vector
}

Is there any way that I can improve my code?

Answer 1

Don't use &String

in function parameters. It has virtually no use. Instead use &str, since it can be implicitly converted from &String, but not the other way around.

Handle errors

You currently happily unwrap every character.to_digit(10), though this may fail if e.g. letters are provided.

Check for valid input

The input should be a string of exactly 9 digits. Your code happily accepts any amount of digits, though the result of their processing will not constitute a valid ISBN.

Represent the ISBN as a struct

So you can have an object that guarantees the representation of a valid ISBN.

Suggested:

Cargo.toml

[package]
name = "isbn"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
clap = { version = "4.3.8", features = ["derive"] }

src/main.rs

use clap::Parser;
use isbn::Isbn;
use std::str::FromStr;

#[derive(Debug, Parser)]
struct Arguments {
    #[arg(index = 1)]
    digits: String,
}

fn main() {
    let arguments = Arguments::parse();

    match Isbn::from_str(arguments.digits.as_str()) {
        Ok(isbn) => println!("The ISBN is: {isbn}"),
        Err(error) => eprintln!("{error}"),
    }
}

src/lib.rs

use std::fmt::{Display, Formatter};
use std::str::FromStr;

const FACTORS: [u8; 9] = [10, 9, 8, 7, 6, 5, 4, 3, 2];

#[derive(Debug)]
pub enum Error {
    InvalidDigit(char),
    InvalidLength(usize),
}

impl Display for Error {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::InvalidDigit(chr) => write!(f, "Invalid digit: {chr}"),
            Self::InvalidLength(len) => write!(f, "Invalid length: {len}"),
        }
    }
}

#[derive(Debug)]
pub struct Isbn([u8; 10]);

impl Display for Isbn {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        for digit in self.0 {
            if digit == 10 {
                write!(f, "X")?;
            } else {
                write!(f, "{digit}")?;
            }
        }

        Ok(())
    }
}

impl From<[u8; 9]> for Isbn {
    fn from(value: [u8; 9]) -> Self {
        Self([
            value[0],
            value[1],
            value[2],
            value[3],
            value[4],
            value[5],
            value[6],
            value[7],
            value[8],
            calculate_checksum(&value),
        ])
    }
}

impl FromStr for Isbn {
    type Err = Error;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        let mut digits = Vec::new();

        for c in s.chars() {
            if let Some(digit) = c.to_digit(10).and_then(|digit| u8::try_from(digit).ok()) {
                digits.push(digit);
            } else {
                return Err(Error::InvalidDigit(c));
            }
        }

        let array: [u8; 9] = match digits.try_into() {
            Ok(array) => array,
            Err(error) => return Err(Error::InvalidLength(error.len())),
        };

        Ok(Self::from(array))
    }
}

fn calculate_checksum(digits: &[u8; 9]) -> u8 {
    let dot_product: u32 = digits
        .iter()
        .zip(FACTORS)
        .map(|(&x, y)| u32::from(x) * u32::from(y))
        .sum();

    for checksum in 0..=10u8 {
        if (dot_product + u32::from(checksum)) % 11 == 0 {
            return checksum;
        }
    }

    0
}

Or, alternatively, if you want to only store the first 9 digits of the ISBN and calculate its checksum on the fly:

src/lib.rs

use std::fmt::{Display, Formatter};
use std::str::FromStr;

const FACTORS: [u8; 9] = [10, 9, 8, 7, 6, 5, 4, 3, 2];

#[derive(Debug)]
pub enum Error {
    InvalidDigit(char),
    InvalidLength(usize),
}

impl Display for Error {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::InvalidDigit(chr) => write!(f, "Invalid digit: {chr}"),
            Self::InvalidLength(len) => write!(f, "Invalid length: {len}"),
        }
    }
}

#[derive(Debug)]
pub struct Isbn([u8; 9]);

impl Isbn {
    #[must_use]
    pub fn checksum(&self) -> u8 {
        let dot_product: u32 = self
            .0
            .iter()
            .zip(FACTORS)
            .map(|(&x, y)| u32::from(x) * u32::from(y))
            .sum();

        for checksum in 0..=10u8 {
            if (dot_product + u32::from(checksum)) % 11 == 0 {
                return checksum;
            }
        }

        0
    }
}

impl Display for Isbn {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        for digit in self.0 {
            write!(f, "{digit}")?;
        }

        match self.checksum() {
            10 => write!(f, "X"),
            checksum => write!(f, "{checksum}"),
        }
    }
}

impl From<[u8; 9]> for Isbn {
    fn from(value: [u8; 9]) -> Self {
        Self(value)
    }
}

impl FromStr for Isbn {
    type Err = Error;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        let mut digits = Vec::new();

        for c in s.chars() {
            if let Some(digit) = c.to_digit(10).and_then(|digit| u8::try_from(digit).ok()) {
                digits.push(digit);
            } else {
                return Err(Error::InvalidDigit(c));
            }
        }

        let array: [u8; 9] = match digits.try_into() {
            Ok(array) => array,
            Err(error) => return Err(Error::InvalidLength(error.len())),
        };

        Ok(Self::from(array))
    }
}

You can even optimize the storage space of the Isbn further by just storing its numerical value, which maxes out at 999999999 and thus fits into a u32:

src/lib.rs

use std::fmt::{Display, Formatter};
use std::str::FromStr;

const FACTORS: [u8; 9] = [10, 9, 8, 7, 6, 5, 4, 3, 2];

#[derive(Debug)]
pub enum Error {
    InvalidDigit(char),
    InvalidLength(usize),
}

impl Display for Error {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::InvalidDigit(chr) => write!(f, "Invalid digit: {chr}"),
            Self::InvalidLength(len) => write!(f, "Invalid length: {len}"),
        }
    }
}

#[derive(Debug)]
pub struct Isbn(u32);

impl Isbn {
    #[must_use]
    pub fn digits(&self) -> String {
        self.0.to_string()
    }

    #[must_use]
    pub fn checksum(&self) -> u8 {
        let dot_product: u32 = self
            .digits()
            .chars()
            .map(|c| c.to_digit(10).unwrap_or_else(|| unreachable!()))
            .zip(FACTORS)
            .map(|(x, y)| x * u32::from(y))
            .sum();

        for checksum in 0..=10u8 {
            if (dot_product + u32::from(checksum)) % 11 == 0 {
                return checksum;
            }
        }

        0
    }

    fn from_digits(digits: &[u8]) -> Result<Self, Error> {
        if digits.len() != 9 {
            return Err(Error::InvalidLength(digits.len()));
        }

        Ok(Self(
            digits
                .iter()
                .rev()
                .enumerate()
                .map(|(exponent, digit)| {
                    u32::from(*digit)
                        * 10u32.pow(u32::try_from(exponent).unwrap_or_else(|_| unreachable!()))
                })
                .sum(),
        ))
    }
}

impl Display for Isbn {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.digits())?;

        match self.checksum() {
            10 => write!(f, "X"),
            checksum => write!(f, "{checksum}"),
        }
    }
}

impl FromStr for Isbn {
    type Err = Error;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        let mut digits = Vec::new();

        for c in s.chars() {
            if let Some(digit) = c.to_digit(10).and_then(|digit| u8::try_from(digit).ok()) {
                digits.push(digit);
            } else {
                return Err(Error::InvalidDigit(c));
            }
        }

        Self::from_digits(digits.as_slice())
    }
}

Here I implemented from_digits() as a private method of Isbn and not via a TryFrom<&[u8]> implementation, since it would need additional checks to guarantee that each digit is in {0..=9} if it was exposed publicly where this prerequisite is not guaranteed.

Answer 2

There's already a very good answer but I see another way this code can be improved

Favor formulas over loops

When calculating the checksum, you currently use a loop once the dot product is found to find a value that makes the sum be a multiple of 11.

You can use the remainder of dot_product % 11 to find that number, because checksum + dot_product ≡ 0 (mod 11), and then mod that by 11 again since you need to convert an exact 11 to 0. It's then safe to cast to a u8 since mod 11 will always return an integer on the interval [0, 10] and u8s are on the interval [0, 255].

You can use this formula, either with return or as the tail of the function, once you've calculated dot_product.

((11 - (dot_product % 11)) % 11) as u8