Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MinMaxXYs method to the Envelope type #418

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Add MinMaxXYs method to the Envelope type #418

merged 2 commits into from
Oct 13, 2021

Conversation

peterstace
Copy link
Owner

@peterstace peterstace commented Oct 11, 2021

Description

Add MinMaxXYs method to the Envelope type

The following pattern was frequently employed by users of the library:

    envMin, minOK := env.Min().XY()
    envMax, maxOK := env.Max().XY()
    if !minOK || !maxOK {
            // envelope is empty, special handling
    }
    // finally do something with envMin and envMax

This works, but has some downsides:

  • There is a lot of "min"/"max" stutter in the first three lines.

  • It feels a little more verbose than it could be.

  • It can be easy for subtle bugs to occur if the mins and maxes don't all line up (e.g. min is used in place of max in a single place).

  • The predicate in the if statement is awkward because minOK and maxOK should always have the same value. It doesn't feel right to ignore one of them though due to the asymmetry. ORing them together isn't quite correct either, since it might imply that they could be different. ANDing them together also implies that they might be different too. The problem is that there are 2 sources of truth for one logical boolean value.

The new method changes this pattern to:

    envMin, envMax, ok := env.MinMaxXYs()
    if !ok {
            // envelope is empty, special handling
    }
    // finally do something with envMin and envMax

There's still a tiny bit of stutter between envMin, envMax, and MinMaxXYs, but there's less chance for bugs to slip in. It's one line shorter, and there are less variables overall, and there is just one boolean so the if statement is much simpler.

Check List

Have you:

  • Added unit tests? Yes.

  • Add cmprefimpl tests? (if appropriate?) N/A

  • Updated release notes? (if appropriate?) Yes.

Related Issue

Benchmark Results

N/A, this change shouldn't have a performance impact since it's a new feature.

The following pattern was frequently employed by users of the library:

	envMin, minOK := env.Min().XY()
	envMax, maxOK := env.Max().XY()
	if !minOK || !maxOK {
		// envelope is empty, special handling
	}
	// finally do something with envMin and envMax

This works, but has some downsides:

- There is a lot of "min"/"max" stutter in the first three lines.

- It feels a little more verbose than it could be.

- It can be easy for subtle bugs to occur if the mins and maxes don't
  all line up (e.g. min is used in place of max in a single place).

- The predicate in the if statement is awkward because `minOK` and
  `maxOK` should always have the same value. It doesn't feel right to
  ignore one of them though due to the asymmetry. ORing them together
  isn't quite correct either, since it might imply that they could be
  different. ANDing them together also implies that they might be
  different too. The problem is that there are 2 sources of truth for
  one logical boolean value.

The new method changes this pattern to:

	envMin, envMax, ok := env.MinMaxXYs()
	if !ok {
		// envelope is empty, special handling
	}
	// finally do something with envMin and envMax

There's still a _tiny_ bit of stutter between `envMin`, `envMax`, and
`MinMaxXYs`, but there's less chance for bugs to slip in. It's one line
shorter, and there are less variables overall, and there is just one
boolean so the if statement is much simpler.
@peterstace peterstace self-assigned this Oct 11, 2021
Copy link
Collaborator

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

LGTM!

@peterstace
Copy link
Owner Author

Thanks for the review!

@peterstace peterstace merged commit a2051ac into master Oct 13, 2021
@peterstace peterstace deleted the min_max_xys branch October 13, 2021 18:25
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