CODEWITHSUNDEEP
c++ooprefactoring
Eric Z
Eric Z

Reputation: 14515

Will you create a private class member to eliminate multi-level function call?

Although I wrote this example in C++, this code refactoring question also applies to any language that endorses OO, such as Java.

Basically I have a class A

class A
{
public:
  void f1();
  void f2();
  //..
private:
  m_a;
};

void A::f1()
{
  assert(m_a);
  m_a->h1()->h2()->GetData();
  //..
}

void A::f2()
{
  assert(m_a);
  m_a->h1()->h2()->GetData();
  //..
}

Will you guys create a new private data member m_f holding the pointer m_a->h1()->h2()? The benenif I can see is that it effectively eliminates the multi-level function calls which does simplify the code a lot.

But from another point of view, it creates an "unnecessary" data member which can be deduced from another existing data member m_a, which is kinda redundant?

I just come to a dilemma here. By far, I cannot convince myself to use one over the other.

Which do you guys prefer, any reason?

Upvotes: 2

Views: 100

Answers (3)

Zeks
Zeks

Reputation: 2385

Whenever I have something like this I usually store m_a->h1() into a variable with a meaningful name at function scope since it's likely to be used again later in function's body.

Upvotes: 0

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726849

The fancy word for this technique is caching: you calculate a two-away reference once, and cache it in the object. In general, caching lets you "pay" with computer memory for speed-up of your computations.

If a profiler tells you that your code is spending a significant amount of time in the repeated call of m_a->h1()->h2(), this may be a legitimate optimization, provided that the return values of h1 and h2 never change. However, doing an optimization like that without profiling first is nearly always a bad sign of a premature optimization.

If performance is not the issue, a good rule is to stay away from storing members that can be calculated from other members stored in your object. If you would like to improve clarity, you can introduce a nicely named method (a member function) to calculate the two-away reference without storing it. Storing makes sense only in the rare cases when it is critical for the performance.

Upvotes: 6

Darth Android
Darth Android

Reputation: 3502

I would not. I agree it would simply things in your contrived example, but that's because m_a->h1()->h2() has no inherent meaning. In a well-designed application, the method names used should tell you something qualitative about the calls being made, and that should be a part of self-documenting code. I would argue that in properly designed code, m_a->h1()->h2() should be simpler to read and understand than redirecting to a private method which calls it for you.

Now, if m_a->h1()->h2() is an expensive call which takes a significant time to compute the result, then you might have an argument for caching as @dasblinkenlight suggests. But throwing away the descriptiveness of the method call for the sake of a few keypresses is bad.

Upvotes: 0

Related Questions