38

In a project, I've found a code like this:

class SomeClass
{
    private SomeType _someField;

    public SomeType SomeField
    {
        get { return _someField; }
        set { _someField = value; }
    }

    protected virtual void SomeMethod(/*...., */SomeType someVar)
    {
    }

    private void SomeAnotherMethod()
    {
        //.............
        SomeMethod(_someField);
        //.............
    }

};

How do I convince my teammates that this is bad code?

I believe this is an unnecessary complication. Why pass a member variable as a method parameter if you already have access to it? This is also a violation of encapsulation.

Do you see any other issues with this code?

Improve this question
6
  • 23
    What makes you think it's bad? Commented Apr 29, 2012 at 15:50
  • @Yannis Rizos, you think it's good? At least this is unnecessary complication. Why pass variable as method parameter, if you already has access to it? This is violation of encapsulation, as well. Commented Apr 29, 2012 at 15:52
  • 2
    Valid points, please edit the question to include them. We can't help you convince your team mates, but we can help you evaluate the code, that's what your question should be about. Commented Apr 29, 2012 at 15:55
  • 9
    I'd rather have a method that can sum up different variables than a method that does the constant 2 + 2. The parameter on the method is for reusability. Commented Apr 29, 2012 at 18:43
  • One point I think is important here is the type of that parameter. If it's a reference type I see no advantage but if it's a value type I think it make sence since if you modifie that variable type the compiler will warn you about those place you broke the code. Commented Feb 13, 2014 at 13:58

10 Answers 10

Reset to default
4

I think this is a valid topic, but the reason you are getting mixed responses is because of the way the question was formed. Personally, I had the same experiences with my team where passing members as arguments was unnecessary and convoluted the code. We'd have a class that works with a set of members but some functions access members directly and other functions would modify the same members through parameters (i.e. use completely different names) and there was absolutely no technical reason for doing that. By technical reason, I mean an example that Kate provided.

I would recommend to take a step back and instead of focusing exactly on member passing as parameters, initiate discussions with your team on clarity and readability. Either more formally, or just in hallways, discuss what makes some code segments easier to read and other code segments more difficult. Then identify quality measures or attributes of clean code that as a team you would want to strive for. After all, even when working on green field projects, we spend more than 90% of time reading and as soon as the code is written (let's say 10-15 minutes later) it goes into maintenance where readability is even more important.

So for your specific example here, the argument I would use is that less code is always easier to read than more code. A function that has 3 parameters is harder for the brain to process than a function that has none or 1 parameter. If there's another variable name, the brain has to keep track of yet another thing when reading the code. So to remember "int m_value" and then "int localValue" and remember that one really means the other is always more expensive for your brain then simply working with "m_value".

For more ammunition and ideas, I would recommend picking up a copy of Uncle Bob's Clean Code.

Improve this answer
4
  • Downvoted after seeing the influence of the book referenced. Commented Aug 18, 2017 at 19:01
  • Although small part of me is very sad that 5 years after writing the answer, you just took away 2 internet points from me, there's another small part of me that's curious if you could provide some references that would be indicative of the (presumably bad) influence that the book that I referenced had. That would seem fair and almost worth those points Commented Aug 23, 2017 at 1:38
  • the reference is myself, seeing personally the influence on other developers. All the motivations behind such books are good, however, by specifying that all code must follow non-standard guidelines (i.e. guidelines that actually serve a purpose), such books seem to engender a loss of critical thinking, which some interpret as cult-like. Commented Aug 23, 2017 at 16:13
  • regarding the brain processing challenges, consider the concept of cognitive load Commented Jun 29, 2018 at 23:18
35

I can think of a justification for passing a member field as a parameter in a (private) method: it makes explicit what your method depends on.

Like you say, all member fields are implicit parameters of your method, because the whole object is. However, is the full object really needed to compute the result? If SomeMethod is an internal method which only depends on _someField, isn't it cleaner to make this dependency explicit? In fact, making this dependency explicit may also hint that you can actually refactor this piece of code out of your class! (Note I assume we aren't talking about getters or setters here, but code that actually computes something)

I wouldn't make the same argument for public methods, because the caller neither knows nor cares about which part of the object is relevant to compute the result...

Improve this answer
7
  • 3
    What would be the remaining implicit dependency? I assumed from your example that _someField is the only parameter needed to compute the result, and we just made it explicit. (Note: this is important. We didn't add a dependency, we only made it explicit!) Commented Apr 29, 2012 at 17:33
  • 13
    -1 If there are no implicit dependences to instance members, then it should be a static member that takes the value as a parameter. In this instance, although you have come up with a reason, I don't think it's a valid justification. It's bad code. Commented Apr 29, 2012 at 21:41
  • 2
    @SnOrfus I actually suggested refactoring the method entirely out of the class Commented Apr 29, 2012 at 22:51
  • 7
    +1. for "...isn't it cleaner to make this dependency explicit?" Abso-freaking-loutely. Who here would say "global variables are good as a general rule"? That is so COBOL-68. Hear me now and believe me later. In our non-trivial code I will, at times, refactor to explicitly convey where class-global variables are used. We have screwed the pooch in many instances by a) arbitrary back-n-forth use of a private field and it's public property b) obfuscating a fields' transformation , by "hiding the dependencies". Now multiply this by a 3-5 deep inheritance chain. Commented Apr 30, 2012 at 14:11
  • 4
    @Tarion I disagree with Uncle Bob on this. Whenever possible, methods should be function-like and only depend on explicit dependencies. (When calling public methods in OOP, one of these dependencies is this (or self), but it's made explicit by the call itself, obj.method(x)). Other implicit dependencies are the object's state; this usually makes code harder to understand. Whenever possible -- and within reason -- make dependencies explicit & functional-style. In the case of private methods, if possible, explicitly pass every parameter they need. And yes, it helps to refactor them out. Commented Jun 6, 2013 at 16:54
34

I see one strong reason for passing member variables as function arguments to private methods - function purity. Member variables are effectively a global state from the point of view, what's more, a mutable global state if said member changes during the method's execution. By replacing member variable references with method parameters, we can effectively make a function pure. Pure functions don't depend on an external state and have no side effects, always returning the same results given the same set of input parameters - thus making testing and future maintenance easier.

Sure it's neither easy nor practical to have all your methods as pure methods in an OOP language. But I believe you gain much in terms of code clarity by having the methods that handle complex logic pure and using immutable variables, while keeping the impure "global" state handling separated within dedicated methods.

Passing member variables to a public function of the same object when calling the function externally would however constitute a major code smell in my opinion.

Improve this answer
2
  • 7
    Supurb answer. Despite hwats ideal, many classes in todays software are bigger and uglier than entire programs were when the phase "Globals are Evil" was coined. Class variables are, to all practical purposes, globals within the scope of the class instance. Having large amounts of the work done in pure functions makes for far more testable and robust code. Commented Apr 30, 2012 at 8:36
  • @mattnz is there any way you could supply a link to Hwat's bibliography or any of his books about ideal programming? I've been scouring the internet and can't find anything on him. Google keeps trying to autocorrect it to "what". Commented Jan 29, 2019 at 4:24
9

If the function is called various times, sometimes passing that member variable in and sometimes passing something else, then it's OK. For example, I would not consider this bad at all:

if ( CalculateCharges(newStartDate) > CalculateCharges(m_StartDate) )
{
     //handle increase in charges
}

where newStartDate is some local variable and m_StartDate is a member variable.

However if the function only ever gets called with the member variable passed to it, that's odd. Member functions work on member variables all the time. They may be doing this (depending on the language you're working in) to get a copy of the member variable - if that's the case and you can't see that, the code might be nicer if it made the whole process explicit.

Improve this answer
2
  • 4
    It doesn't matter that the method is called with parameters other than the member variable. What matters is that it could be called that way. You don't always know how a method will eventually be called when you're creating it. Commented Apr 29, 2012 at 18:15
  • Maybe you should better replace the "if" condition by "needHandleIncreaseChages(newStartDate)" and then your argument does not hold anymore. Commented Jun 6, 2013 at 7:30
3

What no one touched on is that SomeMethod is protected virtual. Meaning that a derived class can use it AND re-implement its functionality. A derived class would not have access to the private variable and thus could not provide a custom implementation of SomeMethod that depends on the private variable. Instead of taking the dependency on the private variable, the declaration requires the caller to pass it in.

Improve this answer
3
  • What you are missing is that this private member variable has public accessors. Commented Apr 30, 2012 at 1:37
  • So you're saying the protected virtual method should take a dependency on the public accessor of a private variable? And you have a problem with the current code? Commented Apr 30, 2012 at 12:20
  • Public accessors are created to be used. Period. Commented May 1, 2012 at 12:42
2

Some things that come to me when thinking about this:

  1. In general, method signatures with fewer parameters are easier to understand; a big reason methods were invented was to eliminate long parameter lists by marrying them to the data on which they operate.
  2. Making the method signature depend on the member variable will make it harder to change those variables in the future, because you not only need to change inside the method, but everywhere the method is called as well. And since SomeMethod in your example is protected, subclasses will also need to change.
  3. Methods (public or private) that do not depend on the class internals do not need to be on that class. They could be factored out into utility methods and be just as happy. They have next to no business being part of that class. If no other method depends on that variable after you move the method(s), then that variable should go too! Most likely the variable should be on it's own object with that method or methods that operate on it becoming public and being composed by the parent class.
  4. Passing data around to various functions like your class is a procedural program with global variables just flies in the face of OO design. That is not the intent of member variables and member functions (see above), and it sounds as if your class is not very cohesive. I think it's a code smell that is suggesting a better way to group your data and methods.
Improve this answer
1

GUI frameworks typically have some sort of 'View' class that represents things drawn on the screen, and that class usually provides a method like invalidateRect(Rect r) to mark part of its drawing area as needing to be redrawn. Clients might call that method to request an update to part of the view. But a view might also call its own method, like:

invalidateRect(m_frame);

to cause redrawing of the entire area. For example, it might do this when it's first added to the view hierarchy.

There's nothing wrong with doing this -- the view's frame is a valid rectangle, and the view itself knows that it wants to redraw itself. The View class could provide a separate method that takes no parameters and uses the view's frame instead:

invalidateFrame();

But why add a special method just for this when you can use the more general invalidateRect()? Or, if you did choose to provide invalidateFrame(), you'd very probably implement it in terms of the more general invalidateRect():

View::invalidateFrame(void)
{
    invalidateRect(m_frame)
}

Why pass variable as method parameter, if you already has access to it?

You should pass instance variables as parameters to your own method if the method doesn't operate specifically on that instance variable. In the example above, the view's frame is just another rectangle as far as the invalidateRect() method is concerned.

Improve this answer
1

This makes sense if the method is a utility method. Say for instance you need to derive a unique short name from several free text strings.

You would not want to code up a separate implementation for each string, instead passing the string to a common method makes sense.

However if the method always works on a single member it seems a bit dumb to pass it as a parameter.

Improve this answer
1

The main reason for having a member variable in a class is to allow you to set it in one place, and have it's value available to every other method in the class. In general therefore you would expect that there is no need to pass the member variable into a method of the class.

I can however think of a couple of reasons why you might wish to pass the member variable into another method of the class. The first is if you need to guarantee that the value of the member variable needs to be used unchanged when used with the called method, even if that method will need to change the actual member variable value at some point in the process. The second reason is related to the first in that you might wish to guarantee the immutability of a value within the scope of a method chain - when implementing a fluent syntax for example.

With all of this in mind, I wouldn't go so far as to say that the code is "bad" as such if you pass a member variable to one of it's class's methods. I would however suggest that it is generally not ideal, as it might encourage a lot of code duplication where the parameter is assigned to a local variable for example, and the extra parameters add "noise" in the code where it isn't needed. If you are a fan of the Clean Code book, you'd know that it mentions that you should keep the number of method parameters to the barest minimum, and only if there isn't another more sensible way for the method to access the parameter.

Improve this answer
0

You are missing if the parameter ( "argument" ) is reference or read-only.

class SomeClass
{
    protected SomeType _someField;
    public SomeType SomeField
    {
        get { return _someField; }

        set {
          if (doSomeValidation(value))
          {
            _someField = value;
          }
        }
    }

    protected virtual void ModifyMethod(/*...., */ ref SomeType someVar)
    { 
      // ...
    }    

    protected virtual void ReadMethod(/*...., */ SomeType someVar)
    { 
      // ...
    }

    private void SomeAnotherMethod()
    {
        //.............

        // not recommended, but, may be required in some cases
        ModifyMethod(ref this._someField);

        //.............

        // recommended, but, verbose
        SomeType SomeVar = this.someField;
        ModifyMethod(ref SomeVar);
        this.someField = SomeVar;

        //.............

        ReadMethod(this.someField);
        //.............
    }

};

Many developers, usually assign directly the internal fields of a variable, in the constructor methods. There are some exceptions.

Remember that "setters" can have additional methods, not just assignament, and sometimes, even be virtual methods, or call virtual methods.

Note: I suggest keep the internals fields of properties as "protected".

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.