Implemented json to ShopItem conversion, Fixes #56 - [merged] #107

Closed SamB440 closed 2 years ago

In GitLab by @jordanplayz158 on Jun 6, 2021, 24:24

Merges master -> master

This pr fixes #56 as said in the title, for information on the changes, check the commit message and/or changes.

I believe StringBuilder should be use for this type of thing

I think we should not have this static and instead have one instance of ShopParser and access it through ShopParser#getShopItems

Use throws ReflectiveOperationException

No star imports

Cache the new ShopParser and use that instance everywhere

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:25

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopItem.java line 42

Why do you think a StringBuilder is necessary? Doesn't seem like it's needed imo (and the ide recommended making the toString method that way)

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:26

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 4

Sorry, The ide keeps doing that

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:30

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 31

Well I thought it would make sense for it to be static as there should only ever be one instance of ShopParser (in use) and the shop parser should always return the most up to date values (and the Sell, Blacksmith, and Food Shops should always get the most updated values as well, makes it helpful and easy for implementing the /taleofkingdoms reload command) but if you think it should be instance based then I will change it.

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:31

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 89

Fixed, will be pushing commit soon.

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:32

Commented on src/main/java/com/convallyria/taleofkingdoms/TaleOfKingdoms.java line 159

Check my reasoning for the static map to understand why I did that, if you think my reasoning is not good enough then I will do your requested change.

Don't think it is necessary, it's just a general pattern that I usually see in Minecraft/Minecraft server code.

This should be ok for now then, might need changing in the future though

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:36

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 31

Alright!

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:39

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopItem.java line 42

Oh, I think it would just make it more messy to have a bunch of appends and I don't feel our needs fits the purpose of StringBuilder, not sure why the Minecraft/Minecraft server code would use StringBuilder but I definitely wouldn't use their code as a good example given how bad minecraft server performance has gotten (since 1.13), but nevertheless, that is interesting.

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:46

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 89

changed this line in version 2 of the diff

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:46

Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 4

changed this line in version 2 of the diff

In GitLab by @jordanplayz158 on Jun 6, 2021, 15:46

added 1 commit