Looking for feedback on simple image to ASCII art program - can it be better? Is it idiomatic code?

I've been working on a simple command-line program that reads an image file and produces some ASCII art as output.

Since I'm learning Rust and this is actually my first program apart from having done the Rustlings exercises, I'd love to get some feedback in particular regarding code style, if there's anything that can be improved etc.

Here is the repo:

Thank you! :slight_smile:

2 Likes

There’s not much code… well easier to review! Anyway here’s a few things I could notice…

This signature is of course more generic than necessary (only being used once with some concrete types), but I guess it’s good for practice of writing such generic function signatures anyway…

fn downsample_image<T>(
    img: &T,
    config: &Config,
) -> ImageBuffer<T::Pixel, Vec<<T::Pixel as Pixel>::Subpixel>>
where
    T: GenericImageView,
    <T as GenericImageView>::Pixel: 'static,
{
    …
}

here’s an idea to refactor this function signature nonetheless, without actually changing its generality, but IMO it’s perhaps nicer (since it’s more compact):

  • using impl Trait can be convenient and easy to read
  • since the return type depends on the Pixel associated type of T, we need an explicit generic arg for that nonetheless
  • giving it its own generic parameter P, constrained with Pixel = P, has the additional benefit of avoiding the need for the unwieldy <T::Pixel as Pixel>::Subpixel syntax

so here’s the result:

fn downsample_image<P: Pixel + 'static>(
    img: &impl GenericImageView<Pixel = P>,
    config: &Config,
) -> ImageBuffer<P, Vec<P::Subpixel>> {
    …
}

    let ratio = img.dimensions().0 as f32 / (config.width / config.amount as u16) as f32;
    let new_width = (img.dimensions().0 as f32 / ratio) as u32;
    let new_height = (img.dimensions().1 as f32 / ratio) as u32;

I see you use floating points for calculating rations. I personally am a fan of using integer arithmetic when floats aren’t really needed, though for images I guess it doesn’t matter. When using floats, it’s generally better to use f64 though, just as a reasonable default; f32’s use case is mainly when you have so many of them that the memory matters.

Another kind of thing you could pay attention to here if you wanted is to use checked arithmetic and checked fallible conversions (e.g. from float back to u32), because your program probably behaves more reasonable if it errors on overflows instead of the default wrapping behavior (in optimized builds).


    img.enumerate_rows()
        // map each row to a string of ASCII characters (terminating with a newline)
        .map(|(_, row)| {
            row.map(|(_, _, lumi)| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
                .collect::<String>()
                + "\n"
        })
        // collect all the rows into a single string
        .collect::<String>()

you use enumerate_rows but don’t make use of the indices… just use rows instead :wink:

    img.rows()
        // map each row to a string of ASCII characters (terminating with a newline)
        .map(|row| {
            row.map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
                .collect::<String>()
                + "\n"
        })
        // collect all the rows into a single string
        .collect::<String>()

    row.map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
        .collect::<String>()
        + "\n"
…
/// Takes a char and return a `String` consisting of that char repeated `n` times
fn repeat_char(c: char, n: usize) -> String {
    std::iter::repeat(c).take(n).collect()
}

allocating a new String like this for each individual character isn’t necessary. You can just flat_map to an iterator of char instead:

    row.flat_map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
        .collect::<String>()
        + "\n"
…
fn repeat_char(c: char, n: usize) -> impl Iterator<Item = char> {
    std::iter::repeat(c).take(n)
}

A new string per line … well I guess that’s less bad, but if you wanted, you can also take the flat_map one step further and only .collect once:

    img.rows()
        // map each row to a line of ASCII characters (terminating with a newline)
        .flat_map(|row| {
            row.flat_map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
                .chain(std::iter::once('\n'))
        })
        // collect all the rows into a single string
        .collect::<String>()

Also, we have (fairly new) the

function, so (in the flat_mapped use case) your helper can be skipped entirely :slight_smile:

    row.flat_map(|lumi| std::iter::repeat_n(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))

Looking at code like this

    let width_mult_factor = match config.mode {
        HorizontalAdjustmentMode::Stretch => config.amount,
        _ => 1,
    };

note that it would generally be preferred to write out the other case explicitly, too.

You’ll gain the guarantees of exhaustiveness checks, so future refactors to HorizontalAdjustmentMode would make the compiler point out all the relevant places in the code you’d need to touch.

Pair this with a consistent ordering of the match arms, i.e.

    let width_mult_factor = match config.mode {
        HorizontalAdjustmentMode::Stretch => config.amount,
        HorizontalAdjustmentMode::Repeat => 1,
    };

and

    let horiz_repeat_count = match config.mode {
        HorizontalAdjustmentMode::Stretch => 1,
        HorizontalAdjustmentMode::Repeat => config.amount,
    } as usize;

and it becomes more clear that these two sections of code are deliberately implementing a different logic.


Another minor thing: It looks like img_to_ascii doesn’t need ownership of its argument (only calling the rows iterator)

fn img_to_ascii(img: image::GrayImage, config: &Config) -> String {

so you can make the argument by-reference

fn img_to_ascii(img: &image::GrayImage, config: &Config) -> String {
    // generate and print ASCII art
    println!("{}", img_to_ascii(&downsampled_image, &config));

It doesn’t matter because you don’t need the ownership in main anymore either, but correctly declaring the ownership story of functions helps not to get confused in any future refactors.

4 Likes

Thank you so much for taking the time to write such a detailed and useful feedback - very much appreciated! :slight_smile:

Just a quick follow-up:

So what's the best practice in Rust for cases such as this one? Is it best to start with a specialised function and only generalise if/when needed or is it better to write something general in the first place? (e.g. I did look a bit into Haskell before and that's the vibe I've got - make everything super generic from the start)

-   let ratio = img.dimensions().0 as f32 / (config.width / config.amount as u16) as f32;
-   let new_width = (img.dimensions().0 as f32 / ratio) as u32;
-   let new_height = (img.dimensions().1 as f32 / ratio) as u32;
+   let ratio = img.dimensions().0 / (config.width / config.amount as u32);
+   let new_width = img.dimensions().0 / ratio;
+   let new_height = img.dimensions().1 / ratio;

haha, no, that’s not what I meant by

In order to not use floats, you’ll first have to re-write the calculation in a way that doesn’t rely on any accuracy of fractional intermediate results… otherwise the floats were necessary…

E.g.

let amount = config.amount as u32;
let new_width = config.width / amount;
let new_height = config.width * img.dimensions().1 / img.dimensions().0 / amount;

for rounding down all results

or perhaps something like

let div_round = |n, d| (n + d / 2) / d;
let amount = config.amount as u32;
let new_width = div_round(config.width, amount);
let new_height = div_round(
    config.width * img.dimensions().1,
    img.dimensions().0 * amount,
);

for rounding to the closest number, I guess…

And then you can still worry about adding overflow checks if you like, I guess.

1 Like

I’d say it’s a matter of personal preference. The main problem of “more generic than necessary” really is just the generally higher effort it takes to write all the function signatures; i.e.

fn downsample_image(img: &GrayImage, config: &Config) -> GrayImage

is just a lot shorter. If you are using only one format of image but want to leave the precise choice in your application to be something more easy to refactor, then another pragmatic approach could be to define a type synonym

type Image = image::GrayImage;

and work with that.

In any case, as soon as you do need the capability to work with different types, generics can be super useful, so especially when practicing the language, it isn’t a bad idea to use them just so you become confident you also are able to do it when it’s necessary.

One aspect of Haskell that’s different is also that generic function signatures can even be inferred. Global type inference, and the lack of method-resolution and coercion features that may hinder it, means that writing generic code is a bit more easy.

Which reminds me… another possible downside of overly-generic functions is that if you use them wrong (accidentally), the error messages become harder to read. E.g. if you use downsample_image with the wrong argument, it’s a more complex error about trait implementations of GenericImageView for the generic version, but a simplet error about the argument not being GrayImage in the non-generic version.

2 Likes

yeah I thought you were saying that since the image dimensions start as integers and end as integers anyway, it wouldn't make sense to go through converting to float etc... which is something I was thinking as well.

Regardless, I definitely wasn't paying attention to the issue of the accuracy of the intermediate steps, thanks for pointing that out!

do you mean using functions such as checked_mul, checked_div etc. and then doing e.g. unwrap on the result so that it will panic instead of giving a wrapped result in case of overflow? Or is it something else entirely?

yes, exactly checked_mul & checked_add etc… (checked_div isn’t really necessary on unsigned, unless you want to subsequently handle division-by-zero without panic). Realistically, overflow shouldn’t happen, the dimensions will never be that large, but nonetheless it seems more reasonable to panic than to silently wrap (because we aren’t calculating this in a loop, so the “performance” of overflow checks don’t matter). I.e. unwrap seems totally fine here IMO.

If you do want nicer error handling beyond that, it seems more reasonable to enforce some stricter size limits anyway… [1] handling e.g. degenerate cases like a huge width parameter; or an original image that’s 1 pixel wide and a million pixels high, could otherwise lead to huge memory consumption when scaling the image and/or computing the output.


  1. e.g. compute target sizes for the re-scaled image, and the final string, and error before their computation if that size is expected to be unreasonably large, like gigabites ↩︎

1 Like