API design question: methods or enum?

code review
1

I'm (kinda') finished my library Kolorwheel.rs, it works, but I don't really like its API. Now it's a bunch of methods, which can be chained, but there're too many of them:

let kw = KolorWheel::new()
    .set_count(self.cols * self.rows)
    .set_hsl(self.color.h, self.color.s, self.color.l)
    .hue_reli(90)
    .sat_reli(-20)
    .sat_offs(&self.sat_offsets[0..6])
    .lit_offs(&self.lit_offsets[0..3]);

Maybe, a few simple methods would be better, which receive its command enum:

let kw = KolorWheel::new()
    .init(SpinInit::SetCount(self.cols * self.rows))
    .init(SpinInit::SetHsl(self.color.h, self.color.s, self.color.l))
    .spin(SpinMode::HueRelativeInclusive(90))
    .spin(SpinMode::SaturationRelativeInclusive(-20))
    .spin(SpinMode::SaturationOffsetsSlice(&self.sat_offsets[0..6]))
    .spin(SpinMode::LightnessOffsetSlice(&self.lit_offsets[0..3]));

As you see, I'm also struggling to choose a name style for the functions, e.g. change hue value relative, including specified target is now reli() vs HueRelativeInclusive() (and we're now not talking about case style). Maybe it's only question of taste, but all opinions are welcome.

My hint is that enum is better, but is it okay for an API, to use enum with lot of items?

The actual API is:

pub fn set_count(mut self, count: u32)
pub fn set_hsl(mut self, h: u32, s: u32, l: u32)
pub fn set_rgb(mut self, r: u8, g: u8, b: u8)
pub fn set_rgb_f(mut self, r: f32, g: f32, b: f32)
pub fn set_rgb_fa(mut self, rgb: [f32; 3])
pub fn set_rgb_hex(mut self, hex: &str)
pub fn hue_abs(mut self, amount: u32)
pub fn hue_reli(mut self, amount: i32)
pub fn hue_relx(mut self, amount: i32)
pub fn hue_vals(mut self, values: &[u32])
pub fn hue_offs(mut self, offsets: &[i32])
pub fn sat_abs(mut self, amount: u32)
pub fn sat_reli(mut self, amount: i32)
pub fn sat_relx(mut self, amount: i32)
pub fn sat_vals(mut self, values: &[u32])
pub fn sat_offs(mut self, offsets: &[i32])
pub fn lit_abs(mut self, amount: u32)
pub fn lit_reli(mut self, amount: i32)
pub fn lit_relx(mut self, amount: i32)
pub fn lit_vals(mut self, values: &[u32])
pub fn lit_offs(mut self, offsets: &[i32])

In the JavaScript version (which has a slightly less feature) I was using sentinel values to ignore H/S/L components, so the number of methods is much less, one method sets all the options, but it's not a quite readable, e.g. (warning: JS code!):

.abs(0, 0, -1, -1, 8)

Another glitch is, that absolute functions use -1 for "none", but relative functions use 0, as they can be negative as well.

I'm also struggling with return value. Now it's a primitive struct:

pub struct Color {
    pub r: u8,
    pub g: u8, 
    pub b: u8,
}

It would be great to return with egui::Color32, but then it's not a universal lib anymore. Also, it would be great to accept egui::Color32 as parameter.

Should I write a kind of "adapter" for egui and for other UIs as well?

Fortunately, HTML/CSS interfacing is easy, string in hex notation format does the job.

Any advice is welcome, this color thing is tripling-by-doubling my troubles (RGB x HSL), not talking about I'm pretty beginner with Rust.

2

In Rust, it's customary to at least reasonably spell out names instead of resorting to abbreviations; this is especially important for the most important component(s) of multi-part names. sat_reli() would have to be saturation_relative_inclusive() or at least saturation_rel_incl() to be considered idiomatic.

According to this, and the fact that you start to write many essentially setter methods, it would probably be best to switch to the enum-based solution you mentioned. That would allow you to have setter methods for each higher-level "concept" as opposed to needing a method for every possible combination of properties.

That said, there are quite a few other issues with your current API:

  • None of the types your crate defines derive any of the standard traits they could. There's no reason why Color couldn't be Debug + Copy or KolorWheel couldn't be Debug + Default.

  • Explicit returns at the end of builder methods and the Iterator::next() method are unnecessary. Rust is an expression language. Just write self or Some(color) or whatever the return value is.

  • If the RGB hex string argument of set_rgb_hex_parse() is invalid, you silently ignore the error. This is the worst possible way to handle errors: to simply not handle them at all. Wherever you need to parse user input, you should instead handle and/or propagate errors by returning Result.

  • Relatedly, parsing of the RGB color struct should be separated from the setter. Ideally, you wouldn't have any "overloads" such as:

    • set_rgb
    • set_rgb_f
    • set_rgb_fa
    • set_rgb_hex`

    Instead, you should have a single setter and have it accept something that can be converted or parsed into a Color, e.g.:

    pub fn set_color<C: Into<Color>>(color: C)

  • You shouldn't store redundant state such as count and countf. If you only need the count as a float, make the count field have type f32. If you need it as an integer, too, then convert it to a float when necessary. The same is true for storing two representations of the color (both RGB and HSL) at the same time.

  • The repetitive groups of fields such as x_spin, x_spin_counter, x_offset, x_offset_counter could (and should) be pulled out to a new struct type.

  • You should probably move the declaration and the impl of UDTs next to each other.

  • The conversion between RGB and HSL color spaces should not be part of KolorWheel; that violates the single responsibility principle. The conversion should be in From impls between separate representations for RGB and HSL.

  • Instead of ubiquitous mutation, you should use value-returning APIs whenever possible. The best example of this is computing the max of the three components in convert_rgb_to_hsl(): instead of mutating a max variable, use the .max() method, which is present on any type implementing Ord.

  • Instead of as-casting, which can silently truncate, change sign, or do other funky things implicitly, use from and/or try_from impls when converting between numeric types.

  • Rather than parsing the hexadecimal string manually, character-by-character, use the from_str_radix method, defined on all integer types.

  • You should run Clippy (cargo clippy) and Rustfmt (cargo fmt) on your code and consolidate any suggestions they may have.

6 Likes
3

One of the primary goals for your API should be to make invalid inputs unrepresentable. That being said, I definitely prefer the use of enums since they give you more control on how they can be built.

You can accept Into<Color> in your API and implement the From trait for egui::Color32:

impl From<egui::Color32> for Color {
  ...
}
1 Like
4

My problem is that any use of egui::Color32 comes with egui dependency, which I don't really want. Maybe someone want to use the lib for a simple web page renderer, without egui.

It would be a solution somehow detecting whehter egui is imported, and render egui-related code only then. Or just add it as build option. (I haven't played with macros yet.)

5

There's no need for macros. What you are looking for are Cargo features and optional dependencies.

4 Likes
6

There were quite a few other non-idiomatic constructs in your code, too, which would be too long to enumerate and explain in detail. Overall, I would write your library like this.

4 Likes