5
\$\begingroup\$

I'm learning Rust.

I find it sometimes confusing when compared to other programming languages, especially when dealing with strings and slices.

Her's an implementation of the 99 beer song that I've written, but the code feels clunky and I think it's bad.

How would you have written it, if it was a production code of a real-world software? Any way to improve performance, readability and a more reasonable way to deal with strings in Rust?

const BOTTLE: &str = "bottle";
const S: &str = "s";
const OF_BEER: &str = "of beer";
const N_UP: &str = "N";
const N_DOWN: &str = "n";
const O_MORE: &str = "o more";
const ON_THE_WALL: &str = "on the wall";
const SPACE: &str = " ";
const COMMA: &str = ",";
const PERIOD: &str = ".";
const NL: &str = "\n";
const TAKE: &str = "Take";
const IT: &str = "it";
const ONE: &str = "one";
const DOWN_AND_PASS: &str = "down and pass it around";
const GO_STORE: &str = "Go to the store and buy some more";
const NINE_NINE: &str = "99";

pub fn verse(n: i32) -> String {
    let mut phrase: String;
    let bottles = &format!("{}{}", BOTTLE, S)[..];
    let bottles_of_beer = &format!(
        "{}{}{}",if n == 1 { BOTTLE } else { bottles }, SPACE, OF_BEER
    );
    let n_str = &format!("{}", n)[..];

    if n < 1 {
        phrase = format!("{}{}{}{}", N_UP, O_MORE, SPACE, bottles_of_beer);
    } else {
        phrase = format!("{}{}{}", n_str, SPACE, bottles_of_beer);
    }

    phrase = phrase + SPACE + ON_THE_WALL + COMMA + SPACE;

    if n < 1 {
        phrase = phrase + N_DOWN + O_MORE;
    } else {
        phrase = phrase + n_str;
    }

    phrase = phrase + SPACE + bottles_of_beer + PERIOD + NL;

    if n < 1 {
        phrase = phrase + GO_STORE;
    } else {
        phrase = phrase + TAKE + SPACE + if n == 1 { IT } else { ONE } + SPACE + DOWN_AND_PASS
    }

    phrase = phrase + COMMA + SPACE;

    if n < 1 {
        phrase = phrase + NINE_NINE + SPACE + bottles;
    } else {
        let n = n - 1;

        if n < 1 {
            phrase = phrase + N_DOWN + O_MORE + SPACE + bottles;
        } else {
            phrase = phrase + &format!("{}", n)[..] + SPACE + if n == 1 { BOTTLE } else { bottles };
        }
    }

    phrase = phrase + SPACE + OF_BEER + SPACE + ON_THE_WALL + PERIOD + NL;

    return phrase;
}

pub fn sing(start: i32, end: i32) -> String {
    let mut s = String::new();
    let mut x = start;

    while x >= end {
        if x < start {
            s = s + NL;
        }
        s = s + &verse(x)[..];
        x -= 1;
    }

    return s
}
\$\endgroup\$
2

1 Answer 1

2
\$\begingroup\$

Being myself new to rust, take the following with a grain of salt. I would welcome further edits.

I think, generally, that there are two things your code is trying to do. On one hand, it's about string tokens and concatenation.
On the other, there's the solving the 99 bottles problem.

99 Bottles

For this problem, your approach seems complex and results in rather gauche code. What makes it so, is that it's hard to understand at first glance what phrase is at any point.
Adding comments would be a good thing.
Myself, I would forego the whole tokenisation part, and solve it in a more concise way.

There are only a handful of sentences in the song, which need slight variations. I'd like to use something like Python's raw strings:

SOLO = r"{QTY} bottles of beer on the wall, {QTY} bottles of beer."
SOLOT.format(QTY=count)

In rust, I managed to get this behaviour by using std::String::replace:

const SOLO: &str = "{QTY} bottles of beer on the wall, {QTY} bottles of beer."
let mut solo: String = SOLO.to_string().replace("{QTY}", &count.to_string());

Once can likewise replace bottles for its singular form, one for it.

Other details to clarify are: 1. are negative bottles allowed? 2. in one place, we're talking of phrase, in another of verse, and returned are a bunch of sentences. Which nomenclature is correct? A single term ought to be used. 3. We're only dealing with beer, perhaps bottles_of_beer could become bottles?

We then end up with something like so:

const SOLO: &str = "{QTY} bottles on the wall. ";
const CHORUS: &str = "Take one down and pass it around, {QTY} bottles of beer on the wall.\n";
const BOTTLE: &str = "bottle"; // for substitution when 1 bottle left.
const NONE: &str = "\
No more bottles of beer on the wall, no more bottles of beer.\n\
Go to the store and buy some more, {QTY} bottles of beer on the wall.\n\
";

pub fn get_song(start: u32, stop: u32) -> String {
    let mut song: String = String::new();

    for x in (start..stop+1).rev() {
        if x == 0 {
            let no_beer: String = NONE.to_string().replace("{QTY}", &start.to_string());
            song = song + &no_beer;
            break;
        }
        let mut solo: String = SOLO.to_string().replace("{QTY}", &x.to_string());
        let mut chorus: String = CHORUS.to_string().replace("{QTY}", &x.to_string());
        if x == 1 {
            solo = solo.replace("bottles", BOTTLE);
            chorus = chorus.replace("bottles", BOTTLE);
            chorus = chorus.replace("one", "it");
        }
        song = song + &solo + &chorus;
    }
    return song;
}


fn main() {
   println!("{}", get_song(10, 0)); 
}

note that I am not sure this is a correct way to do such constant strings.
I think let mut song: String = String::new(); is the preferred style, as seen in the rust book, over let mut phrase: String;

The while loop can be replaced by a for loop. This is actually the textbook example:

for x in (stop..start+1).rev() {
    ...
}

There's the +1 increment to include the starting value, then the iterator is reversed to go from high to low values.

String Tokens and Concatenation

To stay on the string concatenation path, I suggest commenting what each path does. There are several if n < 1, which could be put together. Within that, n is redefined and subtracted, leading to an nested if n < 1 test.

We can observe that there are two types of operations: building different types of sentences, and checking whether we're at the end of the song (n < 1).

Let's agree to not return a phrase, but a paragraph:

let mut paragraph: String = String::new();

// Build qty of bottles on the wall sentence
if n < 1 {
  ...
  paragraph = format!(...);
} else { ... }

// Build qty of bottles left on the wall sentence
if n < 1 {
   paragraph = format!("{} {}", paragraph, ...);
} else { ... }

And so on.

Now I'm thinking, what's actually done here, is not checking a condition, but rather matching a parameter, so if-elses could become:

// Build qty of bottles on the wall sentence
match n {
   0 => paragraph = format!("{}{}{}{}", N_UP, O_MORE, SPACE, bottles),
   _ => paragraph = format!("{}{}{}", n_str, SPACE, bottles), 
}

By using match extensively, we also can remove the nested if-else:

    // Build closing 
    let phrase = match n {
        0 => format!("{}{}{}", NINE_NINE, SPACE, bottles),
        1 => format!("{}{}{}{}", N_DOWN, O_MORE, SPACE, bottles),
        _ => format!("{}{}{}", qty, SPACE, bottles),
    };

Finally, we get to such a result:

const BOTTLE: &str = "bottle";
const BOTTLES: &str = "bottles";
const OF_BEER: &str = "of beer";
const UPPERCASE_N: &str = "N";
const LOWERCASE_N: &str = "n";
const O_MORE: &str = "o more";
const ON_THE_WALL: &str = "on the wall";
const SPACE: &str = " ";
const COMMA: &str = ",";
const PERIOD: &str = ".";
const NL: &str = "\n";
const TAKE: &str = "Take";
const IT: &str = "it";
const ONE: &str = "one";
const DOWN_AND_PASS: &str = "down and pass it around";
const GO_STORE: &str = "Go to the store and buy some more";
const NINE_NINE: &str = "99";

pub fn verse(n: u32) -> String {
    let mut paragraph: String = String::new();

    // Is `bottle` plural in this paragraph?
    let bottles: String = match n {
        1 => BOTTLE.to_string(),
        _ => BOTTLES.to_string(),
    };

    // Create first half of first verse.
    let phrase: String = match n {
        0 => UPPERCASE_N.to_owned() + O_MORE + SPACE + &bottles,
        _ => n.to_string() + SPACE + &bottles,
    };
    paragraph = paragraph + &phrase;
    paragraph = paragraph + SPACE + ON_THE_WALL + COMMA + SPACE;

    // Create second half of first verse.
    let phrase: String = match n {
        0 => LOWERCASE_N.to_owned() + O_MORE,
        _ => n.to_string(),
    };
    paragraph = paragraph + &phrase;
    paragraph = paragraph + SPACE + &bottles + PERIOD + NL;

    // Create first half of second verse.
    let phrase: String = match n {
        0 => GO_STORE.to_string(),
        1 => TAKE.to_owned() + SPACE + IT + SPACE + DOWN_AND_PASS,
        _ => TAKE.to_owned() + SPACE + ONE + SPACE + DOWN_AND_PASS,
    };
    paragraph = paragraph + &phrase;
    // Add punctuation
    paragraph = paragraph + COMMA + SPACE;

    // Create second half of second verse.
    let phrase = match n {
        0 => NINE_NINE.to_owned() + SPACE + &bottles,
        1 => LOWERCASE_N.to_owned() + O_MORE + SPACE + BOTTLES,
        _ => n.to_string() + SPACE + &bottles,
    };
    paragraph = paragraph + &phrase;

    // Finish second verse
    paragraph = paragraph + SPACE + OF_BEER + SPACE + ON_THE_WALL + PERIOD + NL;

    return paragraph;
}

fn main() {
    let start: u32 = 10;
    let stop: u32 = 0;
    let mut song: String = String::new();
    for x in (stop..start+1).rev() {
        song = song + &verse(x);
    }
    println!("{}", song);
}

This is still not too clear code to me, and I'd rather work with the first variant.

On a final note, there were places were you use n_str or format!("{}", n), be mindful of this types of errors. Likewise, typically start/stop or begin/end are used, but try not to mix them up.
Lastly, I had a lot of trouble distinguishing between O_DOWN N_DOWN, N_UP.

\$\endgroup\$
5
  • \$\begingroup\$ About the tokens to be replaced, there's also the pluralization of words (ie. when quantity is 1 ore more than 1) also when the bottles are 0 the phrase will be very different. (negative are not allowed) \$\endgroup\$
    – Zorgatone
    Oct 13, 2019 at 16:24
  • \$\begingroup\$ Any alternative about using format!("{}{}{}{}{}{}{}{}{}{}{}{}{}").... for a cleaner string concatenation? \$\endgroup\$
    – Zorgatone
    Oct 13, 2019 at 16:26
  • \$\begingroup\$ s = s + &s2 should concatenate without using format, doesn't it? \$\endgroup\$
    – Zorgatone
    Oct 13, 2019 at 16:27
  • \$\begingroup\$ Indeed, thanks for the pointer, I've updated the answer. \$\endgroup\$
    – Cyril D.
    Oct 14, 2019 at 8:17
  • \$\begingroup\$ FYI, rust's format string support named parameters like python's format function. See doc.rust-lang.org/std/macro.format.html \$\endgroup\$ Oct 15, 2019 at 5:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.