12
\$\begingroup\$

Should my class pass parameters internally or reference class level scoped variables?

I'm not sure on the best approach or style for procedure calls that have parameters. Should I go with the class level scoped variables?

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        CalculateAmount();
    }

    private void CalculateAmount()
    {
        this.AmountToPay = (this.Salary * (this.ArbitraryTaxRate / 100));
    }
}

Or explicitly pass parameters into a procedure?

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        CalculateAmount(this.Salary, this.ArbitraryTaxRate);
    }

    private void CalculateAmount(float salary, float taxRate)
    {
        this.AmountToPay = (salary * (taxRate / 100));
    }
}

In my contrived example I think the first is clearer, but as a class grows in size and complexity it would make it harder to track what is coming from where.

\$\endgroup\$
  • 2
    \$\begingroup\$ Don't use binary floating point numbers (float/double) for money. Either use integers or decimal. \$\endgroup\$ – CodesInChaos Dec 16 '14 at 9:28
7
\$\begingroup\$

I try to use class level scoped variables unless there is a specific reason not to. My reasoning for this is that it feels cleaner from an API standpoint.

Some of the reasons I would pass explicit parameters:

  • Increase the re-usability of the method.
  • Make it clear to sub-types what variables are required for the method.
  • Decouple the method from the specifics of the containing type.

I would guess (hope) that there is much more reasoned and academic writing on this topic but I'm far enough removed from my collegiate days to have no idea what the name of that topic would be. As such, I tend to choose based on what "feels" right. If a method exists simply to clean up another method and/or encapsulate some bit of logic I tend to use a class-level scoped variable.

\$\endgroup\$
  • \$\begingroup\$ Clean Code by Robert C. Martin spends a whole chapter on this. Functions ideally have no parameters (nomadic), 1 (monadic) or 2 (dyadic). When using 3 (triadic), you should review why and consider grouping them together in another class. Why? If these 3 have an association here, there /might/ be other associations. Params types don't count as they are arrays. Constructors with large amounts of params should be switched to properties and used as DTOs. \$\endgroup\$ – Dominic Zukiewicz Jul 4 '13 at 21:01
  • \$\begingroup\$ I agree - as long as good object-oriented principles (SOLID) are being followed. In many cases, I've run into a God Class with so many class-scoped variables, it's like the global pollution of C/BASIC days past. \$\endgroup\$ – Jesse C. Slicer Dec 14 '17 at 19:33
9
\$\begingroup\$

Don't store what you can calculate:

public class YouOweTheGovernment
{
    public float AmountToPay
    {
        get { return Salary * (ArbitraryTaxRate / 100); }
    }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;
    }
}

Your AmountToPay field is basically a cache, and caching is notoriously problematic. Case in point: even your tiny code here has a cache invalidation bug. If you change the tax rate or salary, you aren't recalculating the amount to pay.

Every time you add a field, ask yourself "does this field store unique information that no other field stores?" If the answer is no, don't create the field. The less state you have, the easier it is to understand your code.

\$\endgroup\$
  • \$\begingroup\$ I see what you're saying and it's something I hadn't considered. Although your code calculates the AmountToPay each time, the salary could still change leaving the same bug? also to change the salary the constructor would need to be called again thus creating a new object? \$\endgroup\$ – Rob Jan 28 '11 at 20:27
  • \$\begingroup\$ You can't call the constructor again for the same object, Rob. If your design must provide for the salary or taxrate to be changed after construction, then make the setter public instead of private. You must make sure that any value dependent on this change (in this case AmountToPay) must change too, to be in a consistent sane state, which this answer provides. Notice that if salary and taxrate shouldn't change after construction, then you'd be better of caching AmountToPay after construction for faster access. \$\endgroup\$ – Orca Jan 28 '11 at 20:48
  • \$\begingroup\$ my comment was poorly worded, I was trying to say that your example would also have the same invalidation bug you mention but that bug doesn't really exist in this example as the salary and taxRate can only be set when the object is created, theses values can never become out of sync with the AmountToPay. I do think the code is cleaner if the AmountToPay is always calculated. \$\endgroup\$ – Rob Jan 29 '11 at 8:17
  • \$\begingroup\$ @Rob: If the salary changed, subsequent calls to AmountToPay would take the new salary into account. There's no caching, so there's no cache invalidation to worry about. \$\endgroup\$ – munificent Jan 31 '11 at 18:28
  • 2
    \$\begingroup\$ @Voulnet: You're not better off caching AmountToPay for faster access. Caching adds complexity to the code. Even though this class is simple now, it may not be in the future, and cache invalidation bugs are some of the hardest to find. The simple solution is to not cache in the first place. If your profiler has revealed performance problems, then it may make sense to cache. In that case, the cached data should be clearly marked as such. \$\endgroup\$ – munificent Jan 31 '11 at 18:30
5
\$\begingroup\$

Since this is Code Review, I will first start with mentioning that you are not validating the logical correctness of your numbers, salary can't be negative, for example (Unless you're an evil employer). You may have left it out to shorten the question, but consider it a reminder.

If the Calculate amount function does just this single line, then a better approach for you is to take its line this.AmountToPay = (salary * (taxRate / 100)); and put it in the constructor as well. If it is one line, why call a function, load a stack frame and store registers? Just inline that line of logic in the constructor itself, this way AmountToPay will be initialized with the values from the constructor's arguments, so its access will be faster because it's a local variable (beating the first example), and the logic that updates the AmountToPay will not need a function call including stacking up arguments (beating the second example).

If, however, you plan on creating more than one constructor, then keep all the shared code in one place (An init() function). For example suppose we wanted another constructor which also took your loans, then you might do this.

YouOweTheGov(float taxRate, float salary, float loans)
{
... Process logic relating to loans..
init(taxRate,salary); // this function updates ArbitraryTaxRate,Salary,AmountToPay
}

Keep the shared logic in one place, don't call a function if it does a very small piece of code at the constructor because of function preparation overhead (and construction will happen a lot, especially when passing or returning objects!), but rather add the code in the constructor itself. If you have more than one constructor then keep all shared code in a function (don't repeat code!)

\$\endgroup\$
  • 2
    \$\begingroup\$ You could also use constructor chaining over an init() method. It'll make you look cool at cocktail parties.csharp411.com/constructor-chaining \$\endgroup\$ – Visionary Software Solutions Jan 29 '11 at 1:47
  • \$\begingroup\$ @Orca; you had to comment init(). Wasn't the OP's method descriptive enough? It isn't self describing enoygh to reveal intent, so anyone new to this code would say 'what's init doing?', waste time seeing its assignments. With OP's, you know instantly. For your other point, the 'overhead' of a 16 bit function call, on a 3Ghz processor..; would you seriously go back to a client and say you think its a performance bottleneck in the system to calculate tax because you are calling a method? Properties ARE methods in the CLR, so you've already called 2 methods by assigning their initial values. \$\endgroup\$ – Dominic Zukiewicz Jul 4 '13 at 21:25
5
\$\begingroup\$

The second CalculateAmount method is evil. If a class invariant is supposed to be that AmountToPay is the amount of tax due for Salary and ArbitraryTaxRate, calling CalculateAmount with anything other than those parameters would break the class invariant. If you want to pass Salary and ArbitraryTaxRate to a method, then that method should either:

  1. Be a function which returns the calculated amount but does not disturb anything in the class, or
  2. Set the backing fields for Salary and ArbitraryTaxRate to the indicated values, and update AmountToPay appropriately.

If you use the first approach, you could opt to make the function static. It's possible that changes in tax rules might require the function to make use of instance variables that weren't in the original parameter list. Such a change could be problematic if the function were public (suggesting that perhaps it shouldn't be static) but shouldn't pose any difficulty if it's private.

\$\endgroup\$
3
\$\begingroup\$

In addition to the suggestions posted already, I'd recommend going a step farther with your CalculateAmount method:

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        this.AmountToPay = CalculateAmount(this.Salary, this.ArbitraryTaxRate);
    }

    private float CalculateAmount(float salary, float taxRate)
    { 
       return (salary * (taxRate / 100));
    }
}

Since this removes the side effect assignment, the constructor reads cleaner and that method can be reused more easily. If you don't want an immutable object, you'll have to recalculate every time the salary and taxRate change. But it looks like you do since you have the setters private.

\$\endgroup\$
1
\$\begingroup\$

I'd also want to add something else that others didn't.

I'd rather remove the private set in all your properties because what private setter really says is that your value of that properties might change in the class somewhere else, which obviously is not the case.

So instead of leaving the wrong intentions you can make all the properties get only and initialize them in the constructor, this way you will be more explicit in your meaning. :)

\$\endgroup\$

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.