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

Removing unnecessary orientation enumerators #9036

Closed
wants to merge 0 commits into from

Conversation

nknize
Copy link
Contributor

@nknize nknize commented Dec 22, 2014

PR #8978 included 4 unnecessary enumeration values ('cw', 'clockwise', 'ccw', 'counterclockwise'). Since the ShapeBuilder.parse method handles these as strings and maps them to LEFT and RIGHT enumerators, respectively, their enumeration counterpart is unnecessary. This minor change adds 4 static convenience variables (COUNTER_CLOCKWISE, CLOCKWISE, CCW, CW) for purposes of the API and removes the unnecessary values from the Orientation Enum.

closes #9035

@nknize nknize added :Analytics/Geo Indexing, search aggregations of geo points and shapes v1.4.3 v1.5.0 v2.0.0-beta1 review labels Dec 22, 2014
@@ -641,11 +646,7 @@ public int compare(Edge o1, Edge o2) {

public static enum Orientation {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs the string/boolean. They can be regular enum values. And the constants can be inside the enum, so that they can be used as if they were additional values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely doesn't require the string. The boolean is a bit trickier as it ripples through other places in the Geometry builder that is being refactored anyway in #8825.

Copy link
Member

Choose a reason for hiding this comment

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

But you can replace wherever the boolean is used with equality to the enum value:

orientation.getValue() -> orientation == Orientation.RIGHT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well not exactly. There's a boolean variable direction that needs to be the opposite whatever orientation is for inner ring arrays only. RIGHT/LEFT maps to false/true. So if orientation is RIGHT and we're dealing with an inner ring direction needs to be true. I went ahead and made the change, it just doesn't read as nicely as carrying the false/true bool with the enum.

@rjernst
Copy link
Member

rjernst commented Dec 23, 2014

LGTM, just one last minor comment.

@nknize
Copy link
Contributor Author

nknize commented Dec 29, 2014

merged from 6d87284

@nknize nknize closed this Dec 29, 2014
@nknize nknize deleted the fix/9035 branch December 29, 2014 22:00
@clintongormley clintongormley changed the title [GEO] Removing unnecessary orientation enumerators Geo: Removing unnecessary orientation enumerators Feb 10, 2015
@clintongormley clintongormley changed the title Geo: Removing unnecessary orientation enumerators Removing unnecessary orientation enumerators Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >enhancement v1.4.3 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GEO] Remove unnecessary orientation enumerations
3 participants