3
\$\begingroup\$

I'm beginning the long process of developing an Adventure Game/RPG and I was hoping to get some review on a resource system I'm coming up with. How it works is, a resource is generic to every resource in the world. Each resource will have a name, a random yield, and a random quality. So I've created an interface for the generic properties and function each resource will have.

Interface

public interface IResource{
    string Name {get;set;}
    string Quality {get;set;}
    int Yield{get;set;}
    string ConvertQualityToString();
    int GetResourceYield();
}

Then I implement the interface in an actual resource base class.

Implementation

public class Resource : MonoBehaviour, IResource{
    public string Name { get; set; }
    public string Quality { get; set; }
    public int Yield { get; set; }

    public string ConvertQualityToString(){
        var val = Random.Range(0, 3);
        string quality;
        switch (val){
            case 1:
                quality = "Good";
                break;
            case 2:
                quality = "Great";
                break;
            default:
                quality = "Poor";
                break;
        }
        return quality;
    }

    public int GetResourceYield(){
        var val = Random.Range(1, 25);
        return val;
    }
}

And finally, I create the specific resource itself and attach it to my game object in the inspector.

Resource Class

public class Granite : Resource{
    void Start(){
        Name = "Granite";
        Quality = ConvertQualityToString();
        Yield = GetResourceYield();
    }
}

Is this the proper way to work this system? Obviously, the system actually does work as when I log the values on startup, Yield and Quality are randomized and the Name for each resource object I create is correct. Is there a better way to do this? Are there things I should change? Thanks in advance.

\$\endgroup\$
5
\$\begingroup\$

A few notes:

  • All IResource properties have public setters. Are you absolutely certain that other code should be able to modify those? I'd recommend making these read-only unless you have specific reasons not to.
  • ConvertQualityToString and GetResourceYield look like internal helper methods. Making them part of the IResource interface creates a very confusing API: what's the difference between resource.Yield and resource.GetResourceYield()? With the way the code is structured, I'd expect these methods to be protected.
  • Are you sure that string is a good type for Quality? If you've got a fixed number of qualities an enum is a more strongly-typed and descriptive choice. And if different qualities have different characteristics then perhaps a Quality class makes more sense. Try to avoid writing 'stringly-typed' code.
  • ConvertQualityToString gives a 33% chance for each quality. Likewise, GetResourceYield always produces yields between 1 and 25. Are you sure that's applicable to all resources and in all circumstances? Should quality and yield even be determined by a Resource instance itself? What if, for example, some enemies should have a higher chance of dropping high-quality resources? Or what if you want to provide player upgrades that increase the average resource yield?
  • What's the point of that Granite class? There's no custom logic in there, and the resource name itself is sufficient to distinguish between different resources.
  • In ConvertQualityToString, there's no need for that local quality variable: you can let each case return a value directly (which also allows you to remove those break statements).
  • ConvertQualityToString is a strange name: it doesn't really convert anything. Inaccurate and confusing names tend to make code more difficult to work with.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the notes. There is going to be more to the system, for example, resources will be havestable, which is why I gave the properties public setters. The Granite class will have some logic in it, for example when a player is mining the granite resource, that's what will change the Yield of that specific resource. As far as the ConvertQualityToString and GetResourceYield methods go, should I just break them out into a helper class? Thanks again for the advice. \$\endgroup\$ – Robert Oct 19 '18 at 15:10
  • 1
    \$\begingroup\$ Harvesting only requires Yield to be public (or perhaps better, a public Harvest method). And unless the mining mechanism for granite is really different from other resources, you don't need a separate class for it. As for those methods, I'd probably leave them where they are, but I'd make them static. Also, it looks like IResourceDeposit would be a more accurate name for how you're using it. \$\endgroup\$ – Pieter Witvoet Oct 20 '18 at 11:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.