3

I'm working on an old large code base with a colleague. The codebase uses a significant number of std::shared_ptr and previous developers had a fondness for long property names (m_first_username for example).

Some methods in our code access a number of those properties so our code tends to be very verbose:

if (m_first_username->isSomethingOrOther() || m_second_username->isOtherOrSomething()...

So to make the code more readable my colleague wants to use more std::shared_ptr & with local scope:

const std::shared_ptr<...> &tmp = m_first_username->returnsASharedPtr() tmp->isSomethingOrOther();

Something I disagree with because of the shared pointer use count.

What is the best way to make this code more readable? Keeping using constant references to shared_ptr, use std::weak_ptr or live with the long lines of code?

ruipacheco
  • 15,025
  • 19
  • 82
  • 138
  • Another option is to just use a `shared_ptr` here as well. I'm guessing this code does not need to be high performance (use of smart pointers is an indication for this), and this will not affect too much – Eyal K. Feb 11 '18 at 10:55
  • If threading is not a concern, I'd just do `auto tmp = m_first_username;` – CookiePLMonster Feb 11 '18 at 11:03
  • 3
    You consider `tmp->isSomethingOrOther()` to be more readable than `m_first_username->isSomethingOrOther()`? Can you explain that logic? – nwp Feb 11 '18 at 11:05
  • 2
    I personally do not see what's wrong with the verbosity of that code - it looks very much like a lot of the production code I've been exposed to in the past 15 years. If you're really set on possibly making the code *less* readable by aliasing then CookiePLMonster's suggestion seems reasonable to me. – Joris Timmermans Feb 11 '18 at 11:05
  • 1
    `m_first_username` looks like a reasonable name. There is no way for introduction of weird temporary reference to shared ptr with meaningless name `tmp` to improve readability. – user7860670 Feb 11 '18 at 11:06
  • 2
    The proper way to rename a variable is `auto &new_name = old_name;`. Expect that to get completely optimized out. – nwp Feb 11 '18 at 11:06
  • In this situation I would favor a raw pointer over a shared pointer *reference* tbh. `auto tmp = m_first_username->returnsASharedPtr().get();`. Unless you actually need to call shared pointer functions I don't see what it adds other than a possible extra redirection. – Galik Feb 11 '18 at 11:12
  • Do you have a business reason (not a technical reason) for changing working and tested code? – Richard Critten Feb 11 '18 at 11:36
  • 1
    Looks like you are simply overusing the shared_ptr, instead of trying to patch your code by doing these tricks, you could wonder why this method should not simply return a reference/raw pointer to the instance. – JVApen Feb 11 '18 at 11:50
  • @nwp I don't. It's a compromise. – ruipacheco Feb 11 '18 at 12:04

1 Answers1

1

As per @nwp's comment -the proper way to alias a variable name locally would be:

auto& v1 = m_first_user_name;

If you want to go the route of the "returnAsSharedPointer" you posted in the question, what you'd want to use in the classes of m_first_user_name and m_second user_name is the standard C++ enable_shared_from_this.

On the whole, though it's primarily opinion-based, I believe you'll find that most experienced C++ developers will find the new code less readable than the old code. There is nothing wrong with long, descriptive variable names.

Joris Timmermans
  • 10,814
  • 2
  • 49
  • 75