wiki:DM/Policy/OntheUseofGlobalFunctions
Last modified 9 years ago Last modified on 03/09/2010 03:27:03 PM

DRAFT of Proposed Policy

Policy Statement on the Use of Global Functions in C++ and Python

All lsst-developed functions will be encapsulated within a class. The sole exception to the Policy is for functions like "Y operator+(X const& x, Y const& y)" which are often best expressed as global functions. Where possible (i.e. where these classes are not purely implementational such as an iterator), these classes will be classes in the Domain Model.

Where the Domain Model is deficient, developers will communicate with the DM System Scientist and System Architect to cause the Domain Model to be updated. To violate this requires a presentation of justification to the SAT and granting of a waiver.

Corollary Policy Statement on the Wrapping of 3rd Party Non-OO Libraries

Use of a non-Object Oriented third party library requires the library's encapsulation in an object oriented adapter interface (aka wrapper). The rest of the DMS will use the adapter interface rather than the functional library interface. This requirement can be met by either using an interface adapter tool such as SWIG or by creating the adapter interface with custom programming. To violate this requires a presentation of justification to the SAT and granting of a waiver.

Timeline to Policy Implementation

These Policies will be in effect starting with DC4 and as of now in the DMS Reference Design. As such, the team is strongly encouraged to move in this direction now, within the constraints of the DC3b schedule. The existing global functions will be required to be changed starting with DC4 at the latest.

Justification

Jeff provided the following justification in email on 16 Feb 2010:

Please forgive this rather lengthy message. I have been attending most of the design reviews and have been looking at the reverse engineered class diagrams quite a bit. I need to raise a global issue that cuts across the entire design. I am seeing a fairly large number of global functions (functions declared outside of class scope) in the design. Some examples (randomly chosen from the Coadd Pipeline):

C++:

     template<typename CoaddPixelT, typename WeightPixelT>
     void addToCoadd(
         lsst::afw::image::MaskedImage<CoaddPixelT,
         lsst::afw::image::MaskPixel,
         lsst::afw::image::VariancePixel> &coadd,
         lsst::afw::image::Image<WeightPixelT> &weightMap,
         lsst::afw::image::MaskedImage<CoaddPixelT,
         lsst::afw::image::MaskPixel,
         lsst::afw::image::VariancePixel> const &maskedImage,
         lsst::afw::image::MaskPixel const badPixelMask,
         WeightPixelT weight
     );

Python:

def psfMatchMaskedImage(referenceMaskedImage, maskedImage, policy):
     """PSF-match a maskedImage to match a reference MaskedImage

I know there are many more, for example Diffim also has global functions defined in Python which seem to handle much of the actual work.

Based on having lots of hard-earned scars on previous projects Gregory and I see this as an issue (and Tim and Robyn concur) for the following reasons:

  • I am a firm believer in the position that being fully object-oriented leads to systems that are more robust, maintainable, understandable, and extendable (especially after initial implementation). I want to emphasize that just coding in C++ and python does not automatically imply an OO design. It has much more to do with what classes you define, what behavior you allocate to them, what public interfaces they provide, and how they interact with each other. (I know this is a religious topic that has been debated by thousands of people for at least a decade and I don't want to re-open a discussion from first principles, you just have to accept that I have my reasons and I think they are pretty well documented in the literature.)
  • I am unsympathetic to arguments about memory efficiency, creation cost, etc. for global functions vs classes, and via the use of statics, singletons, and memory management in our base classes these are non-issues from my standpoint.
  • I see no problem with classes that have no state, they are called stateless automata and are always present in oo designs of complex systems. In fact, it is a common occurrence that what was originally designed as stateless ends up needing state after more use cases are elaborated or design has progressed, and a class has to be created anyway.
  • The idea that a function that operates on class x cannot be a member of class x is simply wrong, there are many valid reasons to create operators that operate on self. It is particularly valuable to allocate this operation to class x if the operation requires knowledge of the internals of x and there are few dependencies on other classes. Particularly with C++, arguments of classes being too heavyweight can usually addressed by a better refactoring of the classes involved or applying a delegation pattern.
  • The idea that a function that works on classes x and y cannot be cleanly allocated to x or y does not necessarily yield a global function. There is almost always a new class z that emerges (e.g. a factory or another domain object different than x or y) that can cleanly hold that function, and at very least this hides the function implementation behind a stable public interface.
  • There are often groups of functions that can be collected into a class because even though stateless, they do related things and you want use several or all of that group of functions in an application (e.g. CoordinateSystem? that has operations for returning coordinates and a project method projecting onto a standardized normal space/curve and back out, so any transformation between a possible set of N CoordinateSystems? requires only a pair of these classes).
  • The domain model is a very rich source of information when making these decisions (it is why we do a domain model) and needs to be updated with new classes as we define them. Because domain models capture the attribute and operation content of a class without a particular implementation, they tend to remain much more stable over time, so a design guided by a domain model tends to stand up to changes over time better.
  • Application developers and extenders benefit greatly from symmetric and uniform designs. When there is a large mix of oo and non-oo code in interfaces, it becomes much harder to discern the intended usage paradigm and the entire design becomes less intuitive. OO design patterns such as factory help a lot in this area. This is particularly important as one moves up the scope from function to class to pattern to framework.
  • Our ability to use tools for standards enforcement, reverse engineering, test generation, cost estimation, etc. is greatly diminished by the extensive use of global functions. So, while we may save some developers some time, we are de-optimizing the other parts of the project in some bad ways.

The bottom line is that I feel there are significant upsides to as much as possible eliminating global functions and no significant downsides. At the same time, I don't want to derail progress on DC3b just to accomplish this change.

Comments on Policy Proposal in Chronological Order

KTL

  • The idea that a function that works on classes x and y cannot be cleanly

allocated to x or y does not necessarily yield a global function. There is almost always a new class z that emerges (e.g. a factory or another domain object different than x or y) that can cleanly hold that function, and at very least this hides the function implementation behind a stable public interface.

Except for functions like "Y operator+(X const& x, Y const& y)". These are often best expressed as global functions. I think there should be a permanent exception for this case.

[Editor's note: this was included in the Policy statement above]

JeffK

Yes, Gregory has proposed to me the same that, that such operators be allowed as an exception to the policy and be part of the coding standard.

AndyB

I'm not sure I agree with the assessment that "much of the actual work" in diffim is defined in global Python routines; the fact that you do suggests that we have different ideas as to what role Python is supposed to play in code design. So perhaps this needs to be an aspect of the discussion.

I've taken the tack that it should be used to express a user-friendly interface to the C++ code. But I can also see how this would scuttle attempts to back-engineer the EA model from the code, since this happens at the C++ level.

JeffK

Perhaps the word "much" is an overstatement, as this was simply an example of a global function. Also, I understand that this python function adapts the C++ code.

That said, making a "friendlier" interface to the C++ code via python is fine, but that doesn't necessarily imply a non-OO python interface. In fact, as I mentioned below, I'd still prefer an OO interface there, hiding the C++ language/implementation nastiness (e.g. you could certainly hide C++ construction, initialization, boost, STL, etc.), but not the domain model.

The python can do processing, sequence operations, etc. as long as the interface presented is OO. I suppose we could cater to astronomers who just can't or won't use an OO API, but I think that becomes less of an issue each day and with each new generation, and I actually think we should encourage this migration. Python is becoming sort of the de facto standard for many astronomy codes, so I think others are headed this way as well.

Also a clarification, the reverse engineering is done for both python and C++ (but not for python generated by SWIG solely to wrap C++).

Hope this clarifies.

DC3 Meeting Note

Short discussion of global functions in Python and C++; follow-up to Jeff's email about global functions and their tendency to weaken OO design. This is not an edict, not a policy, but an RFP: "request for policy." Current design reviews should consider the issue of globals, but only if the code is sufficiently embryonic that no major refactoring is implied.

RussellO

If every function needs to be a class method, I am concerned that our class APIs may become fat and brittle, as described by Meyers "Effective C++" 3rd edition item #23 "Prefer non-friend non-member functions to member functions". If I understand your argument, we are to avoid this by extensive use of extra classes?

Also, here's a case where I would be happy to follow your dictate but don't know how to make it work in C++:

The convolve function. Logically we would prefer convolve be a method on the Kernel class. However, Kernel is a templated polymorphic class and convolve requires an additional template parameter (the type of the target image). This causes problems for C++. This is described in ticket #679, which has a bit more information. It seems counter-intuitive to make convolve a method on a new class when logically it should be a method on Kernel. I suppose the answer is to find some clever C++ trick to make it work as a member of Kernel, or ask for a waiver.

JimB

If every function needs to be a class method, I am concerned that our class APIs may become fat and brittle, as described by Meyers "Effective C++" 3rd edition item #23 "Prefer non-friend non-member functions to member functions". If I understand your argument, we are to avoid this by extensive use of extra classes?

I'd like to second Russell's concerns here.

While I'm willing to admit that one solution is to group such operations in additional stateless classes, that seems at the very least a misuse of C++; generally, that's what namespaces are for. I find it telling that some of the best public C++ libraries (eg boost) - and even the c++ standard library - make extensive use of free functions, even though the libraries are very much object-oriented.

I actually think the tendency towards free functions over small classes should be even bigger in Python than C++, because Python functions are first-class objects themselves, and much of what we are writing Python is script-like.

I don't mean to suggest that there aren't cases in which introducing additional classes would improve our code, but I think the proposed rule for DC4 is *much* too restrictive; it seems in direct conflict with what are in general the best C++ and Python coding techniques. It seems like the domain model perspective of "object oriented programming" is very Java-centric, and I'm not sure it really applies to the languages we are using.

Perhaps this problem should be viewed as a deficiency in how the domain model is extracted from the code rather than a deficiency in the code design?

KTL

The convolve function. Logically we would prefer convolve be a method on the Kernel class.

You could also create a Convolver class, but it's not clear that this would be any better than a namespace, as Jim suggests.

MikeJ

The convolve function. Logically we would prefer convolve be a method on the Kernel class. However, Kernel is a templated polymorphic class and convolve requires an additional template parameter (the type of the target image). This causes problems for C++. This is described in ticket #679, which has a bit more information. It seems counter- intuitive to make convolve a method on a new class when logically it should be a method on Kernel. I suppose the answer is to find some clever C++ trick to make it work as a member of Kernel, or ask for a waiver.

The way that I would do it is to have all images derive from an ImageBase? class that doesn't know, for example, what the underlying data type is. You could have all the access routines go through double, but store the answer as a float if necessary. (You might want a different base class or no base class for integer-type images, where this API is inappropriate.) MaskedImage should also derive from this and deal with the masked pixels appropriately.

Then the kernel.convolve() function would take a reference to the base class, rather than the derived class. Then it would be allowed to be a virtual function, as desired.

A big advantage of this is that it would allow the kernels to hide a lot of their implementation details. Currently the various Kernel classes make public way too much (IMO) of their implementation -- presumably to allow the convolve function to work correctly. In general the kernel classes don't seem to be primarily designed with the "What can a Kernel do?" and "What does a Kernel know about?" models. But rather a "How does a Kernel work?" model, which is much harder to maintain and extend in a clean way.

For example, I had to make a few performance compromises in using the Kernel base class for my ShapeletKernel? class, which I derive from AnalyticKernel?. I don't know if the methods with the compromises are used much in practice, but if they are, I suspect the use of this class will be slower than it should be.

So I'd like to see a pretty big redesign of the Kernel classes to hide more of the implementation details and let people implement the actual actions of a Kernel (like convolve) in whatever way is best for the particular underlying model. Some of this redesign might require a redesign of Image as well, so it's a big change. But I do think that this is one case where the free function is a sign of bad design. (I don't agree with Jeff that _all_ free functions are bad, but I do think they are an indication that one should think about the API, whether it really should be a free function or if making it a member function would be cleaner.)

RayP

While investigating this issue of global functions, I came upon a very interesting article by Scott Meyers explaining the rational behind #23 (which Russell references):

http://www.drdobbs.com/cpp/184401197

After considering this article, I still tend to think that making global functions rare and prefering function/process classes (as in K-T's Convolver example) or otherwise violating #23. My rational is mainly about emphasizing clarity in the interface and taking advantage of the patterns afforded by function classes. Rather than explain that all here, I direct the interested or otherwise distracted readers to a Trac blog that spells it all out at:

http://dev.lsstcorp.org/trac/wiki/RayPlante/OnGlobalFunctions

Beyond that, I observe from the email discussion that the current use of global functions is probably not due to insufficient adherence to/understanding of OO practices. Thus, closer examination of the particular instances should be in order before we consider a style guideline.

JeffK

Well, what I wanted was to stimulate this discussion and request a policy, which certainly does not have to be as restrictive as the one I outlined. What I feared was that the timing for this discussion was not right, in that it might delay progress on DC3b. So, let's continue the discussion, but as a lower priority than design reviews.

I general, I note Ray's comment:

"Beyond that, I observe from the email discussion that the current use of global functions is probably not due to insufficient adherence to/understanding of OO practices. Thus, closer examination of the particular instances should be in order before we consider a style guideline."

That is, further examination of the current use is in order, and that should inform a style guideline which prevents insufficient adherence to/understanding of OO practices. The trick is to define "sufficient adherence".

Now to some of the specific points being made:

First, I don't at all buy the argument in Meyers #23 that the "clearBrowser" function reduces encapsulation, simply because the number of members of WebBrowser? increased by 1. That seems to me to be a very arbitrary and shallow way to assess encapsulation. Encapsulation is the degree to which clients can invoke class behavior without access to and implementational knowledge of the class' private members. The addition of clearBrowser as a member does not by itself give the client any more dependency on the privates of WebBrowser?, provided that the way clearBrowser is implemented is to invoke the 3 public members already defined (clearCache, clearHistory, removeCookies). Nor, since clearBrowser is implemented solely by invoking existing public members, does it break if the private data structures change. As long as the other 3 don't change their interface, clearBrowser doesn't need to change at all. QED, no reduction in encapsulation. Of course, if one implemented clearBrowser to operate directly on the private data, then it would add work if the private data changes.

Next, I do want to point out that Meyers #23 is referring to "convenience functions...[that] can't offer any functionality a [class] client couldn't already get in some other way." That is, these functions do not add functionality to the class methods they invoke, they simply provide clients a convenient way to make fewer calls to the class methods by bundling them.

My biggest concern (which I admittedly didn't point out very well in the first post) is the embedding of core functionality in the functions outside of class scope. A client should still be able get done what it needs to by direct invocation of the class methods, rather than the convenience function.

Thus, I think a reasonable compromise in positions is to restrict the amount of core functionality in such functions, and position them as "invokers" of class methods as purely as possible. That is, restrict them to "convenience functions". I also feel that if such functions are used, declaring them in a namespace is very important (as in Meyers #23).

That said, if they are truly just convenience functions, and there are multiple of them in the namespace, I see little harm in collecting them into a stateless class, and then all the same arguments about graceful evolution to a stateful class if need arises still apply.

KTL

Encapsulation is the degree to which clients can invoke class behavior without access to and implementational knowledge of the class' private members.

This definition is apparently not the one Meyers is using.

The addition of clearBrowser as a member does not by itself give the client any more dependency on the privates of WebBrowser?

True, but it does add one more function that needs to be checked for breakage when you change the privates of WebBrowser?. This, I think, is why Meyers considers it to reduce encapsulation. When this function is a non-member non-friend, it *cannot* break and so need not be checked.

Nor, since clearBrowser is implemented solely by invoking existing public members, does it break if the private data structures change.

But you cannot guarantee this without looking at the implementation.

Next, I do want to point out that Meyers #23 is referring to "convenience functions...[that] can't offer any functionality a [class] client couldn't already get in some other way." That is, these functions do not add functionality to the class methods they invoke, they simply provide clients a convenient way to make fewer calls to the class methods by bundling them.

Any function that is operating on an object through its public interface is such a convenience function, whether it is a member or not.

My biggest concern (which I admittedly didn't point out very well in the first post) is the embedding of core functionality in the functions outside of class scope. A client should still be able get done what it needs to by direct invocation of the class methods, rather than the convenience function.

It can always do so if the convenience function invokes only public methods.

The one danger with these convenience functions that invoke public methods is that there may be a tendency on the part of the class author to expose too much implementation in the public class interface in order to enable such convenience functions when they should really be member functions with private access to the implementation. Turning classes into near-structs by providing getter/setter methods for all private member variables (rather Python-like, actually) does not aid

encapsulation.

MikeJ

The one danger with these convenience functions that invoke

public methods is that there may be a tendency on the part of the class author to expose too much implementation in the public class interface in order to enable such convenience functions when they should really be member functions with private access to the implementation. Turning classes into near-structs by providing getter/setter methods for all private member variables (rather Python-like, actually) does not aid encapsulation.

This is exactly what I think happened with the Kernel functions. It seems like they were designed to provide the needed functionality to allow convolve to be a non-member function.

RussellO

The one danger with these convenience functions that invoke

public methods is that there may be a tendency on the part of the class author to expose too much implementation in the public class interface in order to enable such convenience functions when they should really be member functions with private access to the implementation. Turning classes into near-structs by providing getter/setter methods for all private member variables (rather Python-like, actually) does not aid encapsulation.

This is exactly what I think happened with the Kernel functions. It seems like they were designed to provide the needed functionality to allow convolve to be a non-member function.

It may appear that way to you, but that is not how I designed them. I designed Kernel to meet one main requirement: support convolution of a MaskedImage with a spatially varying kernel. (Obviously it also had to support convolution with an Image and spatially invariant kernels, but the case mentioned was the main driver).

I also prefer classes that one can see inside of enough to figure out what is going on. It's a delicate balance: one does not want to expose implementation details, but on the other hand a completely opaque box can make testing and debugging difficult.

I had hoped to make convolve a member function and would have done so if I could have figured out how to do it. However, I had to add very little to the API to support it as a non-friend non-member function.

You have additional requirements that expand what Kernel is and needs to do. So it probably is time to consider a redesign (at least post-DC3b).

The idea of modifying Image and MaskedImage to abstract the pixel type (to allow convolve to be a member function) is worth exploring. Obviously we have to be careful to preserve fast access.

JeffK

Encapsulation is the degree to which clients can invoke class behavior without access to and implementational knowledge of the class' private members.

This definition is apparently not the one Meyers is using.

Yes, I find his "member-counting" extension to the above definition of little practical use.

The addition of clearBrowser as a member does not by itself give the client any more dependency on the privates of WebBrowser?

True, but it does add one more function that needs to be checked for breakage when you change the privates of WebBrowser?. This, I think, is why Meyers considers it to reduce encapsulation. When this function is a non-member non-friend, it *cannot* break and so need not be checked.

Still not much value. I would not rely on this fact to avoid regression testing clearBrowser if WebBrowser? privates change. Even if clearBrowser uses only the public methods, the order it invokes them might expose a latent order-dependent bug injected by the change, that wouldn't show up with unit testing each WebBrowser? method individually. That is why integration tests are needed, to expose sequence-dependent or cyclic bugs that don't show up in unit tests. It is also why though encapsulation helps, you still have to test for behavioral bugs even if the interface is stable.

Nor, since clearBrowser is implemented solely by invoking existing public members, does it break if the private data structures change.

But you cannot guarantee this without looking at the implementation.

I prefer not to have to look at all to decide, just regression test the entire interface (including the convenience function) if the class changes. In this sense, I think we are more likely to do the right regression testing if the convenience function is a class member, but it probably doesn't matter, if we have policy of automated and comprehensive regression testing that forces a test of the convenience function when the classes it calls change.

Next, I do want to point out that Meyers #23 is referring to "convenience functions...[that] can't offer any functionality a [class] client couldn't already get in some other way." That is, these functions do not add functionality to the class methods they invoke, they simply provide clients a convenient way to make fewer calls to the class methods by bundling them.

Any function that is operating on an object through its public interface is such a convenience function, whether it is a member or not.

Exactly. So, why not make it a member? The point here is that the developer is purposeful at the time he creates the convenience function, so he knows to use only public members (or by definition it is not a convenience function), and then it MUST be a member.

I recognize that developers wanting to extend the interface for their own purposes may want to add global functions to avoid modifying the class, but as we are providing the basic framework, this should be a temporary situation at best in the framework and our applications.

My biggest concern (which I admittedly didn't point out very well in the first post) is the embedding of core functionality in the functions outside of class scope. A client should still be able get done what it needs to by direct invocation of the class methods, rather than the convenience function.

It can always do so if the convenience function invokes only public methods.

Yes.

The one danger with these convenience functions that invoke public methods is that there may be a tendency on the part of the class author to expose too much implementation in the public class interface in order to enable such convenience functions when they should really be member functions with private access to the implementation. Turning classes into near-structs by providing getter/setter methods for all private member variables (rather Python-like, actually) does not aid encapsulation.

Agreed. In that case, the function needs to be a member. It is a judgement call when that line has been crossed, hopefully it should be exposed by design review.

RussellO

If every function needs to be a class method, I am concerned that our class APIs may become fat and brittle, as described by Meyers "Effective C++" 3rd edition item #23 "Prefer non-friend non-member functions to member functions". If I understand your argument, we are to avoid this by extensive use of extra classes?

Also, here's a case where I would be happy to follow your dictate but don't know how to make it work in C++:

The convolve function. Logically we would prefer convolve be a method on the Kernel class. However, Kernel is a templated polymorphic class and convolve requires an additional template parameter (the type of the target image). This causes problems for C++. This is described in ticket #679, which has a bit more information. It seems counter- intuitive to make convolve a method on a new class when logically it should be a method on Kernel. I suppose the answer is to find some clever C++ trick to make it work as a member of Kernel, or ask for a waiver.

KTL

The addition of clearBrowser as a member does not by itself give the client any more dependency on the privates of WebBrowser?

True, but it does add one more function that needs to be checked for breakage when you change the privates of WebBrowser?. This, I think, is why Meyers considers it to reduce encapsulation. When this function is a non-member non-friend, it *cannot* break and so need not be checked.

Still not much value.

Maybe not for the manager/tester :-), but there might be more value for the developer in not having to think about possibly changing implementations of these convenience functions. If you have to think about everything that calls public methods when the private implementation of a class changes, then encapsulation is useless.

I would not rely on this fact to avoid regression testing clearBrowser if WebBrowser? privates change.

Of course not. Tests never go away, unless the functionality they are testing goes away.

Even if clearBrowser uses only the public methods, the order it invokes them might expose a latent order-dependent bug injected by the change, that wouldn't show up with unit testing each WebBrowser? method individually.

Such an order-dependency is of course a change in the (perhaps implicit) public interface to the class.

Nor, since clearBrowser is implemented solely by invoking existing public members, does it break if the private data structures change.

But you cannot guarantee this without looking at the implementation.

I prefer not to have to look at all to decide, just regression test the entire interface (including the convenience function) if the class changes.

While it might be ideal to say "change the implementation, then run the tests, then fix all the resulting failures", I think that it may be more efficient (even just in terms of getting things to compile) and in the long run safer (since test suites are never completely comprehensive) to be proactive about changing things that might depend on the implementation before running tests. Limiting the number of such things that need to be addressed is beneficial.

if we have policy of automated and comprehensive regression testing that forces a test of the convenience function when the classes it calls change.

We should just test it always.

Any function that is operating on an object through its public interface is such a convenience function, whether it is a member or not.

Exactly. So, why not make it a member?

So that it is explicit that it does not depend on private data members. Otherwise, there is no apparent distinction in the interface between convenience functions and private-dependent methods.

The point here is that the developer is purposeful at the time he creates the convenience function, so he knows to use only public members (or by definition it is not a convenience function, and then it MUST be a member).

??? If it only uses public members it need not be a member.

JeffK

It's an excellent discussion but I fear I may be diverting you from more important tasks in the short term, so feel free to post again, but this will probably be my last post on this topic. It is also okay to agree to disagree ;-) Thanks!

Jeff

On 2/18/10 2:20 PM, "Kian-Tat Lim" <ktl@…> wrote:

The addition of clearBrowser as a member does not by itself give the client any more dependency on the privates of WebBrowser?

True, but it does add one more function that needs to be checked for breakage when you change the privates of WebBrowser?. This, I think, is why Meyers considers it to reduce encapsulation. When this function is a non-member non-friend, it *cannot* break and so need not be checked.

Still not much value.

Maybe not for the manager/tester :-), but there might be more value for the developer in not having to think about possibly changing implementations of these convenience functions. If you have to think about everything that calls public methods when the private implementation of a class changes, then encapsulation is useless.

I am certainly thinking of the manager/tester, but even more so, the application developer who uses this design to develop new apps (vs the original developer who created it in the first place). I expect and hope that many developers will apply/test this design many more times than we will create or even modify it.

As long as the function is truly a convenience function (relies only on public methods of this class), the only time one needs to consider a change is when one is already modifying the internals, and then it seems a trivial effort to verify that it is in fact a convenience function, so the interface it uses is still fine. The encapsulation is valuable and works in terms of not having to change the convenience function, which is good (although encapsulation alone never prevents the kind of sequence-dependency of cyclic bugs I mentioned previously).

On the other hand, whatever gain in "encapsulation" we get with a separate function is, in my mind, more than offset by the additional coupling and loss of cohesion in the overall design we get from splitting out a separate function from the class on which it solely depends.

Think of someone learning to use this design. With a separate function, one has to learn the function AND the class interface to know that one can clear a cache, history, etc. individually or all 3 at once. One may even miss the fact there there is a convenience function at all, although using a namespace and ensuring it is declared in the same file would both help here.

It seems to me easier to learn this design if these are all in the class interface. I compare this to a mechanical device design where there are many individual parts with no apparent structuring vs one made of assemblies, although ultimately the same number of individual parts. Groups of parts within a small number of assemblies makes the whole thing easier to learn, without prohibiting learning it at the more detailed level when necessary.

I would not rely on this fact to avoid regression testing clearBrowser if WebBrowser? privates change.

Of course not. Tests never go away, unless the functionality they are testing goes away.

Even if clearBrowser uses only the public methods, the order it invokes them might expose a latent order-dependent bug injected by the change, that wouldn't show up with unit testing each WebBrowser? method individually.

Such an order-dependency is of course a change in the (perhaps implicit) public interface to the class.

It is precisely the implicit case that gives trouble and requires the integration/regression test on top of the unit tests when this is a separate function. Again, placing the function in the class as a member makes it more likely that this test will be included automatically, as then it will be an obvious unit test.

Nor, since clearBrowser is implemented solely by invoking existing public members, does it break if the private data structures change.

But you cannot guarantee this without looking at the implementation.

I prefer not to have to look at all to decide, just regression test the entire interface (including the convenience function) if the class changes.

While it might be ideal to say "change the implementation, then run the tests, then fix all the resulting failures", I think that it may be more efficient (even just in terms of getting things to compile) and in the long run safer (since test suites are never completely comprehensive) to be proactive about changing things that might depend on the implementation before running tests. Limiting the number of such things that need to be addressed is beneficial.

I'm not arguing against that, only observing that one can't rely on it alone.

if we have policy of automated and comprehensive regression testing that forces a test of the convenience function when the classes it calls change.

We should just test it always.

Any function that is operating on an object through its public interface is such a convenience function, whether it is a member or not.

Exactly. So, why not make it a member?

So that it is explicit that it does not depend on private data members. Otherwise, there is no apparent distinction in the interface between convenience functions and private-dependent methods.

Again, the only time this is relevant is when one is modifying the private members, not when one is using them. See my argument about coupling/cohesion vs encapsulation merits above.

The point here is that the developer is purposeful at the time he creates the convenience function, so he knows to use only public members (or by definition it is not a convenience function), and then it MUST be a member.

??? If it only uses public members it need not be a member.

Sorry, closing ) in the wrong place. Should have read:

The point here is that the developer is purposeful at the time he creates the convenience function, so he knows to use only public members (or by definition it is not a convenience function, and then it MUST be a member).

Discussion veers onto a different Topic: Documentation

MikeJ

Think of someone learning to use this design. With a separate function, one has to learn the function AND the class interface to know that one can clear a cache, history, etc. individually or all 3 at once. One may even miss the fact there there is a convenience function at all, although using a namespace and ensuring it is declared in the same file would both help here.

It seems to me easier to learn this design if these are all in the class interface. I compare this to a mechanical device design where there are many individual parts with no apparent structuring vs one made of assemblies, although ultimately the same number of individual parts. Groups of parts within a small number of assemblies makes the whole thing easier to learn, without prohibiting learning it at the more detailed level when necessary.

This actually speaks to a problem I have with much of the current LSST code (since I recently went through the process of trying to figure out how to use a number of the classes to write my code). Too much of the code currently has very poor documentation. If anything, most rely on doxygen \brief comments at the start of the class, and maybe for a few of the methods. This is woefully inadequate. Each class should have a lengthy comment at the start explaining the intent of the class design, a typical use case, potential pitfalls to be aware of, etc. And each method should have at least a \brief and preferably something longer explaining what the method does.

Take Wcs.h for example (to pick on another class that I've had to use recently). There is a short comment at the start of the class:

/// \brief Wcs supports coordinate system transformations between
pixel and world coordinates
///
/// All Wcs (in the FITS sense) coordinate conventions are supported via
/// Mark Calabretta's wcslib package (http://www.atnf.csiro.au/people/mcalabre
)

After that, the only method with a comment is operator bool(), which strikes me as something that most users won't call. (I know I just wrote my code to assume that the Wcs is valid. Perhaps I need to go back and add some assert statements.) And it's not like the method names are so obvious that comments would be superfluous. Consider these three:

Eigen::Matrix2d getLinearTransformMatrix() const;
lsst::afw::geom::AffineTransform getAffineTransform() const;
lsst::afw::geom::AffineTransform linearizeAt(lsst::afw::geom::PointD
const & pix) const;

Some comments to explain the differences between these would be quite helpful. I had to look at the implementation code to determine which one was the one I needed (the third one), and what the return value actually was (e.g. what are the offset parts of the returned AffineTransform?).

I also had no idea that I should be concerned with whether my positions are relative to a subimage or total image when I was writing my code (an issue that has been raised recently regarding Russell's proposal for changing how pixel coordinates work). This is definitely an issue that should be pointed out in a comment somewhere in the Wcs class to help users make sure they call the functions correctly.

So my suggestion here is that rather than argue about how to standardize method vs. non-method functions and other such basically stylistic issues, it would be far more useful to users of the code to standardize and improve the comments. At the very least require each function to have a doxygen comment. But that's no where near sufficient imo. Perhaps part of the code reviews (i.e. the second review -- not the ones you are doing now) should include reviews of the documentation.

RHL

I totally agree about the documentation.

I think that the doxygen statement is better off than you think (it's in the .cc not the .h file for non-inline functions), but it still only provides information at the micro level and we desperately need higher level documentation.

My personal view is that the only way to get this is to spend money (gasp) and hire a professional whose job it is to work with the developers. Note that that let's us off the hook.

MikeJ

I think that the doxygen statement is better off than you think (it's in the .cc not the .h file for non-inline functions), but it still only provides information at the micro level and we desperately need higher level documentation.

Fair enough. I had forgotten that they were there rather than in the .h file. (Less useful for users trying to learn how to use the code, but I guess we are supposed to go to the documentation web page directly.)

However, I still maintain that the documentation for Wcs is woefully inadequate. Most of the method comments are almost tautological or refer to implementation details that are of little use to someone who just wants to use the class. So I still think better documentation by the authors of each class should be encouraged.

And I think saying that we need to hire a professional is a cop out. We don't hire people to write papers for us explaining science results (although perhaps some people should). This is the same kind of process. Just explain what you did to an anonymous person who wants to know about it. That doesn't require professional training. Just a little bit of care and time. I guarantee it will usually save other people more time than it takes you to write the comment.

RHL

had forgotten that they were there rather than in the .h file. (Less useful for users trying to learn how to use the code

It depends on whether you need to read the source too. TAGS files are you friends.

and I think saying that we need to hire a professional is a cop out. We don't hire people to write papers for us explaining science results (although perhaps some people should). This is the same kind of process.

It isn't that I want the pro to write the docs, I want the pro to say, "We need more detail here and here and here; and can you explain this to me; and I'll write the overview with you".

FergalM

However, I still maintain that the documentation for Wcs is woefully inadequate. Most of the method comments are almost tautological or refer to implementation details that are of little use to someone who just wants to use the class. So I still think better documentation by the authors of each class should be encouraged.

Agreed. I've been working on the Wcs class a lot, and I hold my hand up and admit I've not documented is as much as I should. But too much of the documentation elsewhere is as terse as "Implements an object of the Foo class", which really isn't helpful.

MikeJ

However, I still maintain that the documentation for Wcs is woefully inadequate. Most of the method comments are almost tautological or refer to implementation details that are of little use to someone who just wants to use the class. So I still think better documentation by the authors of each class should be encouraged.

Agreed. I've been working on the Wcs class a lot, and I hold my hand up and admit I've not documented is as much as I should. But too much of the documentation elsewhere is as terse as "Implements an object of the Foo class", which really isn't helpful.

Yes. Wcs was meant as a typical example. Not a call out of an egregious case.