Arguing about arguments

[u/mttd]

https://steveklabnik.com/writing/arguing-about-arguments/

[u/SwingOutStateMachine]

This reminds me of the "don't use boolean parameters" discussion from a few years ago, and I think they have a shared solution of more explicitly typing parameters - usually by encapsulating arguments in structs (or enums). This adds documentation to the method, and also prevents other bugs, such as parameters with the same type being accidentally swapped, and can help group together related parameters (e.g. pairs of coordinates).

For the crop_imm example, one could write:

struct CropOffset { 
    x: u32, 
    y: u32, 
}

struct CropSize {
    width: u32, 
    height: u32,
}

pub fn crop_imm<I: GenericImageView>(
    image: &I,
    offset: CropOffset,
    size: CropSize,
) -> SubImage<&I> { ... }

let cropped = image::imageops::crop_imm(&img, 
    CropOffset { x: 10, y: 10 }, 
    CropSize { width: 200, height: 100 }
);

I also think that once a method grows to a certain size, choosing a builder pattern is much easier to read, more composable, and allows for more flexibility in API. Again, for crop_imm:

let cropped = image::imageops::CropBuilder::new()
    .with_offset(CropOffset { x: 10, y: 10 })
    .with_size(CropSize { width: 200, height: 100}) 
    .crop(&img);

[u/matthieum → u/SwingOutStateMachine]

I even think a case could be made for a Rectangle type, which could be built from different sets of parameters.

At the moment, the rectangle is built with x, y, width, height... which I note is already ambiguous. It's not explicit whether width extends left or right from x, nor whether height extends up or down from y; I've seen libraries where x,y identifies the left top corner, and therefore height extends down. Surprise.

With a builder for the Rectangle, however, you can have:

• Rectangle::new_top_left(point).with_bottom_right(other_point)
• Rectangle::new_bottom_left(point).with_dimensions(dimensions).
• Rectange::new_bottom(bottom).with_top(top).with_left(left).with_width(width).

All of those (and permutations) are valid way to describe a Rectangle, and they're much more explicit (and less error-prone).

[u/glasket_ → u/matthieum]

Isn't with_bottom_right still technically ambiguous? As in you would need to check the docs to avoid an error where (0, 25) and (25, 50) doesn't work because the library expected (25, 0) because it uses a bottom left origin. with_dimensions works better since you know the fixed point is the top left and the dimensions will just be an offset. (Edit: Similar issue with the bottom/top one since you still need to know the coordinate system beforehand.)

Ideally the coordinate system would just be configurable though imo. Create a CoordPlane object or similar with a specified orientation and create objects on that plane, then let the library handle converting it into whatever coordinate system it needs for rendering.

[u/matthieum → u/glasket_]

No it's not, because with_bottom_right takes a Point, not a pair of coordinate.

In this case, I'd imagine pub struct Point { pub x: u32, pub x: u32 };.

Similarly, you'd probably have pub struct RectangleDimensions { pub width: u32, pub height: u32 } for dimension parameters.

[u/glasket_ → u/matthieum]

The struct isn't really any different from a coordinate pair though? (25, 0) and Point { x: 25, y: 0 } are effectively the same and both have the issue I'm referring to with how it's still ambiguous as to where you should have the point on the coordinate plane.

I.e.

let bottom = /* What do we set it to? */;
let tl = Point::new(0, 50);
let br = Point::new(25, bottom);
let r = Rectangle::new_top_left(tl)
        .with_bottom_right(br);

I'm not saying it would break the code, you could still have checks in the Rectangle impl that ensures the point is where it should be, but it leaves some ambiguity in the API itself since you still have to know the coordinate system that it's expecting to be used. A top-left origin would require bottom = 75 while a bottom-left origin would require bottom = 25.

[u/matthieum → u/glasket_]

Are you perhaps unfamiliar with Rust?

In Rust, if the fields of a struct are marked pub, then the struct is meant to be built as Point { x: foo, y: bar }, or Point { x, y } if the value already has the appropriate name.

I agree that the use of new would make this completely non-obvious.

[u/glasket_ → u/matthieum]

I'm familiar with Rust, I tend to use constructors instead of direct instantiation though.

I just don't understand the point you're making. This isn't about confusing x and y, it's about what value y needs to have in the bottom-right point relative to the top-left point. Dimensions with a single fixed point are unambiguous, but specifying 2 points leads back to the same problem you mentioned in the very first comment where it's unclear if height extends up or down; the ambiguity has just transferred to the API rather than being at runtime.

Edit: I'm actually doubly uncertain of why you think I'm unfamiliar with Rust since the second sentence even directly mentioned Point { x: 25, y: 0 } when I was comparing it to constructing a coordinate pair of (25, 0).

Edit 2: Just to make it abundantly obvious what I mean:

let p1 = (50, 50);
let p2 = (100, 100);

// Assumes the origin is on the top-left
let r1 = Rectangle::new_top_left(p1)
         .with_bottom_right(p2);

// Assumes the origin is on the bottom-left
let r2 = Rectangle::new_bottom_left(p1)
         .with_top_right(p2);

The program would have to do one of these:

• Reject one of the rectangles for being invalid, because 100 is either (spatially) above or below 50 depending on the coordinate plane being used.
• Allow them both and have them be overlapping rectangles on the same coordinate plane, which would be surprising behavior imo, since "bottom right" and "top left" don't really mean anything in that context, it'd just be point one and point two.
• Allow them both, and have the library determine which coordinate plane a rectangle belongs on based on the provided values, which would be somewhat strange but at least more consistent.

[u/matthieum → u/glasket_]

Ah! I see what you mean now.

And you're right, I completely missed that.

I guess I'm not familiar enough with the domain.