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 an enum to represent building bays #20

Merged
merged 3 commits into from
Dec 3, 2019
Merged

Add an enum to represent building bays #20

merged 3 commits into from
Dec 3, 2019

Conversation

Brett208
Copy link
Member

@Brett208 Brett208 commented Dec 2, 2019

Closes #19

I didn't follow the filename prefix of "Enum" used in 2 other enum header files as I do not prefer it, or would more prefer it as a suffix. Wanted to point it out in case it wasn't obvious that it varied.

@Brett208 Brett208 requested a review from DanRStevens December 2, 2019 02:27
Copy link
Member

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

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

I was thinking about the enum header naming recently. We are not entirely consistent with this.

There is a NonExportedEnums.h header, with other similar constants. I was thinking the new enum could have fit well there, however, when I went looking for it I realized it was in the Outpost2DLL project. That's actually probably the wrong place for that header. Those enums aren't actually exported names. They're completely made up. It probably makes more sense to put that in OP2Helper, given the division of responsibilities.

Or maybe OP2Helper should be merged with Outpost2DLL, perhaps as it's own subfolder. Some of the things provided by OP2Helper are pretty basic, and pretty closely tied to how the game works, particularly in regards to the enums describing built in constants.

At any rate, the header organization, and project organization are perhaps higher level concerns best dealt with in their own issue.


Given the other concerns, I'm not too picky about the name. It might be better with "enum" in there somewhere. In regards to the NonExportedEnums.h header, I always have trouble finding it, because I can never remember the name, only that it has "enum" in it. I've been recently tempted to rename it so "enum" appears at the start, making it easier to find. However, a deeper problem might simply be that it's a bad name, rather than the order of where to place "enum".

I'm happy to defer on file naming issues.

@Brett208
Copy link
Member Author

Brett208 commented Dec 2, 2019

OK, thanks for the comments. Lets open an issue or 2 for what you discussed.

-Brett

@Brett208
Copy link
Member Author

Brett208 commented Dec 3, 2019

Also, I tacked Enum on the end of the filename to ease finding the filename as suggested.

@Brett208 Brett208 merged commit c44dbb6 into master Dec 3, 2019
@Brett208 Brett208 deleted the BuildingBayEnum branch December 3, 2019 00:15
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.

Provide enum for Bay index
2 participants