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

Refactor CompoundTag reading with a functional syntax #905

Merged
merged 37 commits into from May 21, 2018

Conversation

@Pr0methean
Copy link
Contributor

@Pr0methean Pr0methean commented Apr 27, 2018

We can now check a tag's type and process it if it's the expected type, all with one method call and one copy of the tag-name literal.

Copy link
Member

@aramperes aramperes left a comment

There's a lot of stuff here, I'll do a more extensive review later.

owner.set(GlowPlayerProfile.getProfile(tag.getString("SkullOwner")).join());
} else if (tag.isCompound("SkullOwner")) {
owner.set(GlowPlayerProfile.fromNbt(tag.getCompound("SkullOwner")).join());
if (!tag.readString(

This comment has been minimized.

@aramperes

aramperes Apr 28, 2018
Member

I think flipping the key (string) and the function params for "read*" methods would make their usage more readable.

This comment has been minimized.

@Pr0methean

Pr0methean Apr 28, 2018
Author Contributor

Then they can't be variadic, since varargs have to come last. I'll look into how often I'm actually using them with more than one key argument, now that the conversions are done.

@yannicklamprecht
Copy link

@yannicklamprecht yannicklamprecht commented Apr 28, 2018

I like it how Java 1.8 can cleanup code using consumables.

@Pr0methean Pr0methean changed the title Refactor compoundTag reading with a functional syntax Refactor CompoundTag reading with a functional syntax Apr 30, 2018
contents = Material.getMaterial(tag.getInt("Item")).getNewData((byte) contentsData);
tag.readInt("Item", itemInt -> item[0] = Material.getMaterial(itemInt));
}
if (item[0] != null) {

This comment has been minimized.

@mastercoms

mastercoms May 20, 2018
Member

I'm not sure if I like this. Could readX allow for chaining with andThen or similar (maybe we need a method for failures as well). Then if it's still necessary, we could move the bool that returns if the tag exists to a new method.

}
} else {
tag.readString("EntityId", name -> spawning = EntityType.fromName(name));
if (spawning == null) {

This comment has been minimized.

@mastercoms

mastercoms May 20, 2018
Member

This could also be chained.

carried.setData((byte) compound.getShort("carriedData"));
}
entity.setCarriedMaterial(carried);
if (carried[0] != null) {

This comment has been minimized.

@mastercoms

mastercoms May 20, 2018
Member

Chaining could be beneficial here as well.

@@ -124,8 +134,12 @@ public void mergeInto(CompoundTag other, boolean overwrite) {
////////////////////////////////////////////////////////////////////////////
// Simple gets

public boolean getBool(String key) {
return containsKey(key) && getByte(key) != 0;
public boolean getBoolDefaultFalse(String key) {

This comment has been minimized.

@mastercoms

mastercoms May 20, 2018
Member

Could we just have getBool(String) and getBool(String, boolean)?

This comment has been minimized.

@Pr0methean

Pr0methean May 21, 2018
Author Contributor

Done; see getBoolean(String, boolean).

}
if (world == null && compound.isString("World")) {
world = server.getWorld(compound.getString("World"));
final World[] world = {null};

This comment has been minimized.

@mastercoms

mastercoms May 20, 2018
Member

Chaining might be able to be done here, not sure if it would be messy though.

}
}
} catch (IOException e) {
server.getLogger().log(Level.SEVERE,
"Failed to read structure data from " + structureFile, e);
}
}
if (data == null) {

This comment has been minimized.

@mastercoms mastercoms merged commit 6d5c079 into GlowstoneMC:dev May 21, 2018
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants