Skip to content

Conversation

@mo271
Copy link
Contributor

@mo271 mo271 commented Nov 18, 2024

to catch wrong right == 0 checks.

To change the number like this catches for example a wrong implementation only looking if right_result is zero, but not checking if we are doing a division.

fn eval(e: Expression) -> Result<i64, String> {
    match e {
        Expression::Op { op, left, right } => {
            let left_result = eval(*left)?;
            let right_result = eval(*right)?;
            // Correct line commented out. (Or even better match inside the match below)
            //if (right_result == 0) && (matches!(op, Operation::Div)) {
            if right_result == 0 {
                return Err(String::from("division by zero"));
            }
            Ok(match op {
                Operation::Add => left_result + right_result,
                Operation::Sub => left_result - right_result,
                Operation::Mul => left_result * right_result,
                Operation::Div => left_result / right_result
                })
        }
        Expression::Value(value) => Ok(value),
    }
}
```

to catch wrong `right == 0` checks.
op: Operation::Sub,
left: Box::new(Expression::Value(3)),
right: Box::new(Expression::Value(4)),
right: Box::new(Expression::Value(0)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3-0 = 3, so this isn't a very interesting case of subtraction.

I guess the idea here is to uncover the bug where any expression with 0 in the RHS is considered an error? I'm not convinced that's worth covering in an exercise like this, but maybe it could be done in another test case, rather than modifying this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was the idea. I'm also not sure if we needs this tests, but it has come up during an real world session...
Added as separate test, but if that makes this file too long, perhaps it is not worth adding them.
In that case: no hard feelings, just close this pr...

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I figured this actually came up :)

If we get feedback that this is annoying long, we can remove it.

@djmitche djmitche enabled auto-merge (squash) November 18, 2024 16:05
@djmitche djmitche merged commit 6148cae into google:main Nov 18, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants