1

Note: there is a similar question that addresses my subject: Better style for member variables?

... but that question does not address "dealing with large legacy code base and comprehension of functions that grew too large". I am looking for confirmation or rejection of whether my situation warrants it to be a is a good enough reason to adopt a practice of writing internal-to-the-class functions as if they were separate entities, with their appropriate parameters and return values.

My question

Variables internal to a class are considered to be scoped as "globals" within contains of that class. As such, these variables do not need to be included explicitly as parameters to the class' internal functions. Further, if internal functions return values that may belong to existing inside-class variables, those also do not need to be returned explicitly. You can just refer to them via using this object. But, is doing so a good practice?

My question comes from the context of refactoring a huge legacy codebase. As such, when I see a function inside a class that goes like $this->IamACute3000LineFunctionDoingInsanelyCuteStuffInASpaghettiCodeLikeWay(), I have little clue as to which in-class "global" variables it needs to do its job, and what exactly it returns. This makes it hard and time consuming to deal with that code when any work is to be done on the class. Thus, even though it may not make sense inside the class itself, I do see some merit in writing code like below, treating variables and parameters as if the function was written to be used outside of the class itself:

class HelloClass
{
   $private $name;
   $private $greeting;

   public function __construct($name)
   {
       $this->name = $name;

       //line of interest
       $this->greeting = $this->prepGreeting($this-name);
   }

   //function of interest
   private function prepGreeting($name)
   {
      return "Hello, $name";
   }

   public function getGreeting()
   {
       return $this->greeting;
   }
}

$sayHello = new HelloClass("Dennis");
print $sayHello->getGreeting();

Here, prepGreeting is the function in question. Surely, it looks a bit ridiculous... as it barely takes advantage of built-in class mechanisms, if at all. It can be rewritten as such instead (with appropriate other constructor changes as well, which are not shown):

   private function prepGreeting()
   {
      $this->greeting = "Hello, " . $this->name;
   }

But then you have to actually read the function's body to understand what it does, what it touches and what it returns.

My question is this:

in my example, where I am dealing with large code where understanding a function takes time, I think there is a good reason to write internal class functions as if they were separate, not belonging to the class, as it will be superior for understanding what goes on inside the function, and what it returns. This function will be self-contained and as such, it will help a lot with refactoring. I could pluck that function out, bend the way I need it to bend, move it to another class, if I have to, thus making it easier to work with the class itself. But, in general, would you recommend such function-writing practice when writing/designing classes?

Or would you recommend instead to use built-in this object, treating internal to the class variables as "class globals", even though doing so may impact readability/understanding/comprehension of the function if its size does grow, as it often does in real life projects?

Finally, am I missing any greater points while focusing on this specific function-writing-style issue?

Add-on

I do see merit in perhaps using some middle-ground, where function parameters are not passed to the function (function uses this object to get to those), but the function still returns values, instead of updating this object directly, and it is up to the caller to update this variable. i.e. this will change my "line of interest" to this code: $this->greeting = $this->prepGreeting(); and at least I will know what the function is supposed to return, at a glance.

2

"When I see a function inside a class, yada yada..." I think the important part to remember here is that the function is inside the class. All of the relevant functionality is therefore confined with in the class ("encapsulation"). Your use of the words "global within the class" is not really relevant, in that respect.

All sane object-oriented languages work this way; they expect you to be able to manage the class's internal members satisfactorily, so long as the internal details of the class don't leak out. If working with the internal members of a class has become difficult, that means that the class has become too large and too complex. It probably needs to be divided up into smaller classes.

1

As part of your refactoring efforts, encapsulating functions so that they perform their function with only parameters passed in is very useful.

It decreases coupling. In a large (especially in a large) class, with lots of fields, you have to consider them as globals, mostly because their accessibility and possibility of change are not local to the screen you see in front of you or the method you see in front of you. Moving from sharing "global variables" to passing parameters is a substantial improvement.

If you can reduce or remove side effects while you are at it, you address a similar aspect of coupling. Your function only returns a value or only calls an external method that is not dependent on the host class and you no longer have to manage your globals in the same way.

0

Finally, am I missing any greater points while focusing on this specific function-writing-style issue?

Yup.

By the sounds of it the classes are way to big. Way way to big. If you can't figure out what a private method is doing just by looking at it, or what private variables are being called and what they are used for, it is a serious code smell that your classes are far to big and un-focused.

It should be obvious from the private method that it is updating the state of the object, and the object should have so little state that it should be obvious what it means to say that its state is being updated. If you have an object that has a whole ton of state that can be changed in different ways and you never know what sub-section of the state a method is going to be altering, again your objects are too big.

Of course you might not be able to control this if it is legacy code that you have inherited. But rather than re-factoring the methods it would make more sense to break up the classes themselves. Even your class above could be broken up into a Name class and a Greeting class, in which case the state of the objects becomes clearer (a Name class simply has the name, a Greeting class simply has the greeting)

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.