Cleanup Saddleback Search

[sozelfist]

Checklist:

• I ran bellow commands using the latest version of rust nightly.
• I ran cargo clippy --all -- -D warnings just before my last commit and fixed any issue that was found.
• I ran cargo fmt just before my last commit.
• I ran cargo test just before my last commit and all tests passed.
• I added my algorithm to the corresponding mod.rs file within its own folder, and in any parent folder(s).
• I added my algorithm to DIRECTORY.md with the correct link.
• I checked COUNTRIBUTING.md and my code follows its guidelines.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.44%. Comparing base (857c73a) to head (928c630).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #824   +/-   ##
=======================================
  Coverage   95.43%   95.44%           
=======================================
  Files         315      315           
  Lines       22809    22813    +4     
=======================================
+ Hits        21768    21773    +5     
+ Misses       1041     1040    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sozelfist]

The added edge cases are just awesome.

[sozelfist]

Can you have a look, @vil02?

[vil02]

Suggested change

	fn is_sorted(matrix: &[Vec<isize>]) -> bool {
	pub fn is_sorted(matrix: &[Vec<isize>]) -> bool {

Imagine that you have some matrix. You know that you will query the position of many elements and you want to check if the matrix is soared only once.

[sozelfist]

I understand this point. By the way, do you know how can we express the tests here? Suppose, we have two functions, one for validate matrix, one for saddleback search like this:

fn validate_matrix(matrix: &[Vec<isize>]) -> Result<(), MatrixError> {
    // Check if all rows have the same length (rectangular matrix).
    if matrix.iter().any(|row| row.len() != matrix[0].len()) {
        return Err(MatrixError::NonRectangularInput);
    }

    // Check if matrix is sorted.
    if !is_sorted(matrix) {
        return Err(MatrixError::NotSorted);
    }

    Ok(())
}

and

fn saddleback_search(matrix: &[Vec<isize>], element: isize) -> Option<(usize, usize)> {
    if matrix.is_empty() || matrix.iter().all(|row| row.is_empty()) {
        return None;
    }

    let mut left_index = 0;
    let mut right_index = matrix[0].len() - 1;

    while left_index < matrix.len() {
        match element.cmp(&matrix[left_index][right_index]) {
            Ordering::Equal => return Some((left_index, right_index)),
            Ordering::Greater => {
                left_index += 1;
            }
            Ordering::Less => {
                if right_index == 0 {
                    break;
                } else {
                    right_index -= 1;
                }
            }
        }
    }

    None
}

Can you propose how we can write tests, @vil02?

[vil02]

I would create a function checking if the input is rectangular and simply use it in both is_sorted and saddleback_search.

Regarding testing strategy: it should be enough to check saddleback_search (as it is now) and is_sorted.

[sozelfist]

Can you suggest the detailed changes?

[vil02]

I think we need to check if the input is rectangular.

[sozelfist]

We can move the logic that checks the matrix form and is_sorted into a single function, for example, validate_matrix

[vil02]

As above: I would introduce a new function checking if the input is rectangular and use it in both public functions.