2
\$\begingroup\$

I have a few resources set in my class – coins, diamonds, hearts, enum of these resources names, and method for increasing specified resource. I want to make single "Increase" method for all resources that wouldn't have switch statement. Sort of single-line not overloaded "generic" method independently on which property to increase. Below is the best what I achieved so far. Please advice on best approach to be taken for the described Increase method.

public enum ResourceType { Coins, Diamond, Hearts };

private static int Coins { get { return resourceBox[ResourceType.Coins]; } }
private static int Diamonds { get { return resourceBox[ResourceType.Diamonds]; } }
private static int Hearts { get { return resourceBox[ResourceType.Hearts]; } }
private static Dictionary<ResourceType, int> resourceBox = new Dictionary<ResourceType, int>();

private void Init()
{
    resourceBox.Add(ResourceType.Coins, 0);
    resourceBox.Add(ResourceType.Diamond, 0);
    resourceBox.Add(ResourceType.Hearts, 0);
}

public static void Increase(ResourceType _res, int _income)
{
resourceBox[_res] += _income;
}

However this leads to situation where Dictionary is a primary resource storage, which may not be desired in future. I wonder what could be another (not obligatory dictionary) way to create Increase method as described above.

Improve this question
\$\endgroup\$
5
  • \$\begingroup\$ Could add the entire class definition? It's hard to comment on that small piece of code. \$\endgroup\$ – t3chb0t Mar 22 '17 at 18:36
  • \$\begingroup\$ Well, that's basically almost it, except for a few methods: code public class Resources {} /code \$\endgroup\$ – Madcode Mar 22 '17 at 18:47
  • \$\begingroup\$ code public static void Spend(ResourceType _res, decimal _price) { switch (_res) { case ResourceType.Coins: Coins -= _price; break; case ResourceType.Diamond: Diamonds -= _price; break; case ResourceType.Hearts: Hearts -= _price; break; default: break; } if (OnResourceChanged != null) OnResourceChanged(_res, _price); } \$\endgroup\$ – Madcode Mar 22 '17 at 18:58
  • \$\begingroup\$ And a few similar methods with ugly switches. \$\endgroup\$ – Madcode Mar 22 '17 at 19:00
  • \$\begingroup\$ Using a Dictionary is just an implementation detail which isn't exposed, if the time came to change it, you could do so without changing any other code that uses the Increase function. So what's the problem? \$\endgroup\$ – 404 Mar 22 '17 at 19:12
1
\$\begingroup\$

I find there is nothing wrong with using a dictionary here. What I'm worried about are those properties:

private static int Coins { get { return resourceBox[ResourceType.Coins]; } }
private static int Diamonds { get { return resourceBox[ResourceType.Diamonds]; } }
private static int Hearts { get { return resourceBox[ResourceType.Hearts]; } }

Why do you need them? They are not only static but also private. If you cannot access them from the outside how are you going to use them?

You didn't include the complete class definition but just a small snippet so it's hard to give you more feedback.

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Static method Increase is used outside of this class, so I had to change properties to static as well in order for Increase to have access to them. Sorry, can't show the whole class because of characters limitations. The main responsibility of Resources class is storing and changing those private properties. \$\endgroup\$ – Madcode Mar 22 '17 at 19:37
  • \$\begingroup\$ @Madcode can't show the whole class because of characters limitations ;-) this means there is so much wrong with this class if it doesn't even fit into a question. You really need to split it up into smaller pieces. \$\endgroup\$ – t3chb0t Mar 22 '17 at 19:39
  • \$\begingroup\$ Removed spaces, now it's ok) Don't mind decimals and int types. Really now properties don't need to be static as they are not called from Increase method directly. Here is snippet of how class looked before dictionary approach: public static void Spend(ResourceType _res, decimal _price) switch (_res) case ResourceType.Coins: Coins -= _price; break; case ResourceType.Diamond: Diamonds -= _price; break; case ResourceType.Hearts: Hearts -= _price; break; default: break;} And few other methods of same repetitive structure. So I wanted to avoid this bulk switches. \$\endgroup\$ – Madcode Mar 22 '17 at 20:05
  • \$\begingroup\$ @Madcode we on Code Review have a character limit of 65535 characters. \$\endgroup\$ – Heslacher Mar 23 '17 at 5:39

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.