Wondering if there is a better way to handle panic!

[Squint]

In the following code, 'point' is returned, but to avoid the 'possibly not initialized error' I initialized it to a ridiculous value and then invoked panic! with a bool. I don't want the caller of this function to handle an empty point since it should never be empty, so I use panic! in hopes of satisfying the compiler.

Is there a better way to implement panic here so I don't need the (-1,-1) initialization?

This function takes an x, y mouse coordinate and checks if it falls within any of 64 rectangles:

fn get_clicked_square_id(x: f64, y: f64, scalar: usize) -> (i32, i32) {
    let mut counter = 0;
    let x2 = scalar;
    let y2 = scalar;
    let x = x as usize;
    let y = y as usize;
    let mut point = (-1, -1); 
    let mut square_not_found = true;

    for x1 in (0..456).step_by(scalar) {
        for y1 in (0..456).step_by(scalar) {
            counter += 1;
            println!("value of iterator is: {}> ({}, {})", counter, x1, y1);
            if x1 < x && x < x2 && y1 < y && y < y2 {
                point = (x1 as i32, y1 as i32);
                square_not_found = false;
                break;
            }
        }
    }
    if square_not_found {
        panic!("Square ID not found");
    }else {
        point
    }
} 

[skysch]

The easy thing to do would be just return (x1 as i32, y1 as i32); when you find the point. Then just panic if you ever leave the loop at the bottom.

fn get_clicked_square_id(x: f64, y: f64, scalar: usize) -> (i32, i32) {
    let mut counter = 0;
    let x2 = scalar;
    let y2 = scalar;
    let x = x as usize;
    let y = y as usize;

    for x1 in (0..456).step_by(scalar) {
        for y1 in (0..456).step_by(scalar) {
            counter += 1;
            println!("value of iterator is: {}> ({}, {})", counter, x1, y1);
            if x1 < x && x < x2 && y1 < y && y < y2 {
                return (x1 as i32, y1 as i32);
            }
        }
    }
    panic!("Square ID not found");
} 

[skysch]

I'll also note that the loops here seem really inefficient. It seems like you're just computing the smallest multiples of scalar that are less than x and y. You should be able to just divide x by scalar, round away any fractional part, and then multiply by scalar to get the correct value for x1. And again for y1.

fn get_clicked_square_id(x: f64, y: f64, scalar: usize) -> (i32, i32) {
    if x > 456.0 || y > 456.0 { panic!("No Square ID") }
    let scalar = scalar as f64;
    (
        (x.div_euclid(scalar) * scalar) as i32,
        (y.div_euclid(scalar) * scalar) as i32,
    )
} 

[gbutler69]

Why would you make this a panicking function instead of making it a fallible function that returns a Result<Point,Error>? This seems like trying to use panic in an entirely inappropriate way that runs counter to proper, idiomatic Rust. Were I code reviewing it in a work environment, I would reject it.

[Cerber-Ursi]

And if the input values of function should never trigger a panic due to the logic of surrounding code (i.e. if the panic would really be a bug), the more semantically meaningful might be to use unreachable!.

[Squint]

@gbutler69 Would you please show me what the code look like doing it the idiomatic way? It would be truly educational. A fine example of right and wrong.

[gbutler69]

EDIT: Improved version using a single range instead of literally twice if the "if" of the match arm.

@Squint

Would you please show me what the code look like doing it the idiomatic way?

I'd recommend something similar to what @skysch posted above, but, changing it to use Option instead of Panic. Something like this (Playground Link):

@skysch

pub struct SquareId { pub x : u32, 
                      pub y : u32 }
                      
                      
fn main() {
    // ...something something that gets or computes the input x, y, tile_size, and range values ....
    let x = 10_f64;
    let y = 100_f64;
    let tile_size = 50_u64;
    let max_tiles = 20_u32;
    // .....
    
    // ... this will panic if x and y don't correspond to a valid Square ID
    let SquareId{ x, y } = get_clicked_square_id( x, y, tile_size, max_tiles ).unwrap();
    println!("Found Square ID: {}, {}", x, y );
    
    // ... this will print a message if x and y don't correspond to a valid Square ID
    
    // prints the values because in range
    let x = 10_f64;
    let y = 100_f64;
    match get_clicked_square_id( x, y, tile_size, max_tiles ) {
        Some(SquareId{x,y}) => println!("Found Square ID: {}, {}", x, y ),
        _ => println!("Square ID NOT Found for {}, {}", x, y)
    }

    // prints the not found message because out-of-range
    let x = 1001_f64;
    let y = 100_f64;
    match get_clicked_square_id( x, y, tile_size, max_tiles ) {
        Some(SquareId{x,y}) => println!("Found Square ID: {}, {}", x, y ),
        _ => println!("Square ID NOT Found for {}, {}", x, y)
    }
    
    // this panics on the unwrap because out-of-range
    let SquareId{ x, y } = get_clicked_square_id( x, y, tile_size, max_tiles ).unwrap();
    println!("Found Square ID: {}, {}", x, y );

}

fn get_clicked_square_id(x: f64, y: f64, tile_size: u64, max_tiles: u32 ) -> Option<SquareId> {
    let scalar = tile_size as f64;
    let range = 0f64..(scalar * max_tiles as f64);
    match (x,y) {
       ( x, y ) if range.contains(&x) && range.contains(&y) => Some( SquareId{ x: (x.div_euclid(scalar) * scalar) as u32,
                                                                               y: (y.div_euclid(scalar) * scalar) as u32 }),
      _ => None
    }
} 

[Squint]

@gbutler69
Wow! That is amazing code! Thank you so much!

[Squint]

Thank you so very much for your great help on efficiency...very good.