CODEWITHSUNDEEP
c#oop
Ralt
Ralt

Reputation: 2206

OOP conventions - single function methods

In OOP and all programming in general I know that it is a clear rule that a method should only carry out one specific task, and if another task needs to be carried out then another method should be created. My problem is this seems pointless in my situation as it is just using more code opposed to it being 1 big method.

Here are the 2 ways in which my code could be structured (one which follow standard convention, and another which doesn't but saves code)

Scenario 1 (2 methods)

Increasing stat method

public void increaseStat(short value, string stat)
{
       switch (stat)
       {
           case "stamina":
               if (staminaCheck.Checked == true)
               {
                   stamina += value;
                   staminaText.Text = Convert.ToString(stamina);
               }
               staminaCheck.Checked = false;
               break;

//There are 5 other similar cases for different stats but to save time I removed them
       }

and decreasing stat method

public void decreaseStat(short value, string stat)
{
       switch (stat)
       {
           case "stamina":
               if (staminaCheck.Checked == true)
               {
                   stamina -= value;
                   staminaText.Text = Convert.ToString(stamina);
               }
               staminaCheck.Checked = false;
               break;

//There are 5 other similar cases for different stats but to save time I removed them
       }

As you can see the code is exactly the same apart from 1 operation which is + instead of - . So instead of copying all the code into another method for the sake of conventions I decided to do this

Scenario 2 (1 big method which tackles increasing and decreasing)

public void alterStat(short value, string stat, bool operator)
{
       switch (stat)
       {
           case "stamina":
               if (staminaCheck.Checked == true)
               { 
                   if (Operator == true) stamina -= value;
                   else stamina += value

                   staminaText.Text = Convert.ToString(stamina);
               }
               staminaCheck.Checked = false;
               break;

//There are 5 other similar cases but to save time I removed them
       }

This way I am adding 2 extra lines into each case by adding a simple if statement, but saving a load of code being copied.

I just want you opinions on whether this would be seen as good programming practice or not.

Thank you for your time.

Upvotes: 1

Views: 204

Answers (4)

Nazar Merza
Nazar Merza

Reputation: 3454

The optimal solution is if you can keep good things from both worlds, because you need both of them. The catch is that you make a separation between the public interface that you expose, and the internal implementation (one of the basic concepts in OOP). How do you do it?

You can still have the two methods, increase and decrease the way the are, and then refactor the common code into a private method that each of increase and decrease methods call with the appropriate flag.

public void increaseStat(short value, string stat){
    alterStat(value, stat, true);
}

public void decreaseStat(short value, string stat){
    alterStat(value, stat, false);
}


private void alterStat(short value, string stat, bool operator){
    // do the actual work here.
}

This is a very important pattern, in designing classes. Again, the idea is the separation of what you expose as the class interface, and how you really implement it. Each should serve its purpose, and both can work together.

Upvotes: 1

Desty
Desty

Reputation: 2776

From my point of view, the gain of having much less duplicated code clearly far outweighs the rule of separation of concerns, particularly when it comes to code maintenance.

A similar scenario is using an Enum as a state flag of an object. The proper OOP way would be to model each state as a seperate class in a state-hierarchy. The downside of the latter is the code you have to write to find out what state an object has, particularly if you use a Visitor pattern (which you should when using OOP).

The rule you mentioned is especially useful if it comes to the interface of a class. If it is clearer (for the user of your class) to have IncreaseStat() and DecreaseStat() operations, instead of having a general ChangeStat(), nobody hinders you to implement a private ChangeStat() method which is called by the corresponding public methods for increasing and decreasing a stat. This way you take both advantages: Having specific operations for specific tasks while internally there is no duplicated code.

Upvotes: 2

Ptharien's Flame
Ptharien's Flame

Reputation: 3246

Since C# has lambdas, neither of these solutions is ideal. Instead, I would try to pass in the operation as a function:

public void modStat(string stat, Func<short, short> op)
{
   switch (stat)
   {
       case "stamina":
           if (staminaCheck.Checked == true)
           { 
               stamina = op(stamina)

               staminaText.Text = Convert.ToString(stamina);
           }
           staminaCheck.Checked = false;
           break;

      // more cases
   }

This is also a lot more general than before. Ideally, we could also abstract over properties the same way, but that's much more difficult (it requires something known as a lens, and C# doesn't have much support for lenses).

Upvotes: 0

Tigran
Tigran

Reputation: 62265

Can do something like:

public void Change(string stat, bool decrease)
{
       switch (stat)
       {
           case "stamina":
               if (staminaCheck.Checked == true)
               {
                   if(decrease)
                      stamina -= value;
                   else
                      stamina += value;
                   staminaText.Text = Convert.ToString(stamina);
               }
               staminaCheck.Checked = false;
               break;

//There are 5 other similar cases for different stats but to save time I removed them
       }
}

So create a method that charges to decrease or increase the value passed. What it does is based on bool parameter.

This would be one of possible "shapes" of what you do in second case, but with slightly different solution.

Upvotes: 2

Related Questions