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

Feature/areaRepo #16

Merged
merged 6 commits into from Oct 17, 2019
Merged

Feature/areaRepo #16

merged 6 commits into from Oct 17, 2019

Conversation

JessieVela
Copy link
Contributor

I know we talked about possibly not using an AreaRepo, so take a look and see if it will work. In the world.java I added an example of its use.

…toring and retrieving Areas. Also, added example use of AreaCollection in world. Added a Junit test for the AreaCollection class.
@JessieVela
Copy link
Contributor Author

Resolves #4

@JessieVela JessieVela changed the base branch from master to develop October 14, 2019 21:49
@JessieVela
Copy link
Contributor Author

Just realized I made a pull request to master; Changed to develop and fixed a merge conflict.

@Muirrum
Copy link
Owner

Muirrum commented Oct 15, 2019

I'm a bit worried about the added complexity. Maybe we should make Areas a possible way of organizing? What do you think?

@JessieVela
Copy link
Contributor Author

It does add another layer of complexity. The benefit is you can now add locations to areas and when displaying the possible locations to a user; you can find the locations grouped together for easier processing.
Example:
areas.addAreaObject(new Area("Castle Throne", 1));
locations.buildLocation(coord1, "Room 2", "Cafeteria where everyone eats", areas.searchAreaByName("Castle"));

You created the area "Castle Throne" with a unique ID of 1 and all locations will inserted into "Castle Throne". Now when I display the possible locations I can just find the area and iterate through the locations.

@Muirrum
Copy link
Owner

Muirrum commented Oct 15, 2019

Yeah, that makes sense. How about making them an optional part of making locations? I can think of a few scenarios where Areas don't make sense (single-area short games, single-room world sections, etc.)

@JessieVela
Copy link
Contributor Author

True. If you feel more comfortable shelving it for now i'm all game for that. It does add a layer of complexity and we could revisit it if we start to find a need for.

@Muirrum
Copy link
Owner

Muirrum commented Oct 15, 2019

I think we just need to make it optional, add a constructor option without an Area argument.

@JessieVela
Copy link
Contributor Author

Sounds good. I'll make the changes for it to be optional.

@Muirrum Muirrum self-requested a review October 15, 2019 16:06
@Muirrum Muirrum added enhancement New feature or request in progress Currently being worked on labels Oct 15, 2019
Copy link
Owner

@Muirrum Muirrum left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just do the thing I talked about upthread, and I don't know if the .idea/ folder needs to stay in, but I'll trust your judgment on that. I made a few comments as well, just change those and it should be good!

@JessieVela
Copy link
Contributor Author

Fixed the changes.

@restyled-io restyled-io bot mentioned this pull request Oct 16, 2019
@Muirrum Muirrum self-requested a review October 16, 2019 17:13
Copy link
Owner

@Muirrum Muirrum left a comment

Choose a reason for hiding this comment

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

App.java throws an error (https://scans.gradle.com/s/tgstkxv72ehsm). Looks like you forgot an import in it.

@Muirrum Muirrum added this to the Release Alpha 0.1.0 milestone Oct 16, 2019
@JessieVela
Copy link
Contributor Author

Fixed the error. It was a good catch my local machine ran without throwing the error.

Copy link
Owner

@Muirrum Muirrum 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 to me!

@Muirrum Muirrum merged commit 6b57bec into Muirrum:develop Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress Currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants