Hi Peter

my comments in bold ;-)

Peter Dimov wrote:

Hi Atul,

I happen to have a copy of C++ Coding Style and looked at Item 42. The summary says:

“Don’t volunteer too much: Avoid returning handles to internal data managed by your class, so clients won’t uncontrollably modify state that your object thinks it owns.”

Yes
The example from the book:

class String {
char* buffer_;
public:
char* GetBuffer() const {return buffer_;}  // bad: should return const char*
// …
};

I don’t see how your patch addresses this issue!

The patch addressed the general issue of not exposing the innards instead let the object expose a service…

But see NodeAddress::hostName(), NodeAddress::hostPort() etc…. and setters for them…
There is one other example there

class Socket {

public:

int GetHandle() const { return handle_; } // avoid this
private:

int handle_;

};

Generally it would be very rarely that we return a pointer or reference to the internal representation.

The item advises against returning any internals .. not only pointers or references
We do return copies but that’s quite fine.

but why does the client need to know?  Won’t it be simpler that it asks the object to perform some service?
Now regarding the patch, your parse() function is NodeAddress is better than my implementation in one of the ctors. But why do you want it public?

As it is called from outside… But yes we should strive to make it private ;-)
Let’s keep it private and have the parsing ctor call it. If you need to parse a new string, then create a new object and copy it:

NodeAddress address;
…
address = NodeAddress (ipPortString);

It’s better style to avoid operations with side effects. That’s why for example QByteArray::left() does not modify the objects but instead returns a new one. This design concept is used pretty much and in my opinion pretty well in Qt.

Sure that is a good design principle. 
Regarding PackedHostCaches, your version of readData() is better ;-). My only suggestion would be that readProperty() be renamed to readHostCache() because you have a ‘\n’ delimited list of hosts caches of the form address&properties.

I totally agree with you  ;-)
What is the problem with Private being used as a pure struct?

Because IMHO, the code is not simpler…. 
It’s used to store the private data and allow for easy addition of reference counting for example.

It can still do that with encapsulation..
In my opinion writing two constructors for Private is overkill and due to its design it is NOT subject to feature envy.

The advantage is the address is constructed and not constructed and assigned

I think we already discussed private data and feature envy but we can reopen the discussion.

I think I could not remember this guideline that time ;-) But yesterday as I was skimming thru, I came upon it ….

—cheerio atul

atul wrote:Dear all

C++ Coding Standards by Herb Sutter and Andrei Alexandrescu have this important guideline…

Much of our code seems to violate this… ( I think we briefly discussed this ). I always thought this a major design concern in our code.

There is a lot of scope for removing feature envy too…

Here is the patch and a general idea….

— cheers atul