Fixes from code review on crafting system

[guilhermehto]

I believe this should fix the issues pointed on the reviews :)

As for using packed scenes instead of the jsons, I tried it and it added more confusion to the code. By having them as keys for our dictionaries and separating them from the json, we would have to instance them before accessing their properties, and then once again convert the json to a dictionary to access the recipe. If we went down this route I don't see the need for jsons, but then again we tried this approach in the beginning and creating recipes became a chore so going back isn't right.

I created a small function to return the dictionary from a json in Game.gd. If someone decides to use this system in their game it's easy to do the same in their Utils.

Though I don't discard the chance that I'm being dense and not seeing something obvious 🙂. Even though I didn't want to spend more time in this system as it's already taking a lot of time to get ready.

@razcore-art @NathanLovato

[razcore-rad]

That's fine, I think we can live with the JSON thing. But I think you should pay more attention to certain things.

I don't know if the review system work so I'll leave the comments here.

Game.gd#L33: you should really start using JSON.parse. Remember we're making these examples for an audience, not for ourselves. If Godot offers a better alternative now (eg. RandomNumberGenerator vs global rand_* functions) then use the better alternative. JSON.parse is the better alternative.

~CraftingStation.gd#L48: you're using up the resources, but you're not increasing the amount of crafted items. edti: I mean this isn't a problem if the system work like this, we just need to be sure the others understand why it's doing this. Maybe explain/make it clear in the docstring that the items aren't placed back in the inventory. You know what tripped me up (apart from being tired)? Visually the item appears, but the inventory only looses the resources, for some reason I expected the amount of crafted items to go up too.~ Oh well, just ignore this one

CraftingStation.gd#L53: amount is never used in the function, why bother with it in the first place?

[CraftingStation.gd#L9](ht uest-3/pull/12/files#diff-78cf3d1970d016d6b7d6097c150c3963R9): what's up with these names? CraftingStation.can_craft(item_to_craft... doesn't that just scratch your ears the wrong way? You think this: Crafting.Station.can_craft(item... isn't enough? So ya, use consistent names in all places, if it's craft(item..., don't say can_craft(item_to_craft..., that's inconsistent.

I think this covers it. So the thing is, I could have made all these changes, but we're giving you the opportunity to learn some new things by bugging you with all these small things. It's up to you if you see it as a learning opportunity or not, but expect to be bugged constantly if you're gonna repeat these minor inconsistencies :)

[guilhermehto]

On it, thanks.

CraftingStation.gd#L48: you're using up the resources, but you're not increasing the amount of crafted items.

There's a loop right above it that appends a new item to the crafted items based on the amount

[razcore-rad]

On it, thanks.

CraftingStation.gd#L48: you're using up the resources, but you're not increasing the amount of crafted items.

There's a loop right above it that appends a new item to the crafted items based on the amount

yes I know, it's just that when you run the example I think would be nice to have a visual representation of what happens when you press enter, I mean other than the sword appearing. Like maybe have a text label with "CRAFTED" appearing. What do you think?

[guilhermehto]

This last commit should wrap things up