2

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?

4
  • What is there Java tag ? Dec 17, 2012 at 2:11
  • @Bhavik, Like I said, it's a question also applying to Java development:)
    – Eric Z
    Dec 17, 2012 at 2:13
  • There is an OOP tag and a Language-Agnostic tag.
    – Joe
    Dec 17, 2012 at 2:15
  • @Joe, Thanks. Should I remove Java and C++ tag and use those tags instead?
    – Eric Z
    Dec 17, 2012 at 2:17

3 Answers 3

6

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.

3
  • I believe the OP is looking to optimize readability, not performance
    – Karthik T
    Dec 17, 2012 at 2:19
  • Yes, the performance is not that critical in my case. So you usually just use the direct function calls?
    – Eric Z
    Dec 17, 2012 at 2:20
  • @dasblinkenlight Thank you, now it addresses the issue better. I would like to add though. I have had this situation as well, where the multiple reference was actually a list lookup (small list) but the ultimate pointer was static for the life of the program. In this case I dont think there is a significant downside to doing caching even for the moderate gain, if it is static for the life of the program.
    – Karthik T
    Dec 17, 2012 at 2:32
0

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.

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.