Implemented json to ShopItem conversion, Fixes #56 - [merged] #107
Comments
|
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 |
|
No star imports |
|
Cache the |
|
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
|
|
In GitLab by @JordanPlayz158 on Jun 6, 2021, 15:46 Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 89 Added in pushed commit. |
|
In GitLab by @JordanPlayz158 on Jun 6, 2021, 15:46 Commented on src/main/java/com/convallyria/taleofkingdoms/common/shop/ShopParser.java line 4 Fixed in pushed commit. |
|
In GitLab by @JordanPlayz158 on Jun 6, 2021, 15:46 resolved all threads |
|
approved this merge request |
|
mentioned in commit bed17c3 |
|
requested review from @SamB440 |
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.
The text was updated successfully, but these errors were encountered: