PSA: A couple of notes on API design and usage

[Ben Goodger (Google)]

I've been working through a revamp of the Views API (more on that later), and here are a few general thoughts I've had as I've scanned through uses of it in the codebase:

- The API is too big, there are too many vaguely (but not quite) identical ways of doing similar seeming things

- There is little control over the API upward (protected:) in the class hierarchy as well as outward (public:)

- Roles for specific types aren't clearly defined enough.

- There are some functions that are dangerous, misleading or just silly, and they're exposed on the public API and are then [mis]used by people who aren't aware of the real reason for their existence!

- Repetition is GOOD.

I'm trying to solve some of this. Specifically the lessons learned are:

It's important to think of how and why someone would subclass your class, when determining what is public, protected and private. The default should be for the least visibility.

For example, far too many methods on View are public, even though there is no reason for anyone to call them outside of the framework. Event handlers are a good example. I will be making all of these protected. Processing functions that exist only in the framework should be private. There are cases where people override some of these framework processing functions. In pretty much every case the people doing this have not understood the underlying framework and actually been doing something wrong/buggy.

I think it is preferable to have friendships or private interfaces with internal:: types than to make methods that only internal:: types call public and pollute the standard API.

For example, RootView is a type that not everyone needs to know about. It should be an implementation detail of the view hierarchy. Mostly, people just want to communicate with the containing Widget. RootView thus moves to the internal namespace, and knows how to communicate with the Views privately.

It is important to note the intended clients of particular functionality. It is best to assume that functionality is not generic if only one client needs it, especially if the implementation of that functionality is complex.

For example, View contains some complex logic intended to support clipping of native child views to the visible bounds of their ancestor views. The only legitimate client for this functionality I've found is NativeViewHost, another class in the Views framework. I am currently working to ensure it is the only one that can use this functionality.

It is a good idea for classes to do one thing and do it well.

RootView is currently the only cross platform per-Widget object. As a result it has a lot of routines on it that aren't really related to event propagation... they're just there because it was simpler to put them there than duplicate code across the various Widget implementations. In my redesign, RootView is focused on the internal details of event propagation, and there is a new cross platform Widget class that handles the rest.

Repetition, Repetition, Repetition! Do I need to say it again?

If you feel like you're off the beaten track when you're writing code, you're probably doing something wrong, or the API isn't well developed enough and you should try and fix it.

For example, in Views there are established patterns for View ownership, setup, hierarchy, etc. We have tried to make it really easy to achieve a certain set of functionality. We have not always made it easy enough. As one example, it's still too hard to port, and as another there are still some oddities with native widget focus that seem to leave us littered with hacks all over the place. It is worth understanding the underlying problem, what the framework is doing, and attempting to fix the framework generically than bandaiding in an increasing number of places.

When your code looks like other code, it's likely to raise fewer eyebrows and be easier to review.

-Ben

[Peter Kasting]

On Thu, Feb 17, 2011 at 7:56 AM, Ben Goodger (Google) <b...@chromium.org> wrote:

It's important to think of how and why someone would subclass your class, when determining what is public, protected and private. The default should be for the least visibility.

Another thing to remember along this line is that subclasses can expose APIs the parent had intended to be more private.  For example, if a parent class makes Foo() a protected function, and a subclass overrides it with a public function, outsiders can now reach Foo() via an instance of the subclass.

For this reason, you should pretty much never give an overriding function greater visibility than the base class version.

But in my own code I go even further than this.  I find it beneficial to give everything minimum visibility, which means that a parent public function might be overridden by a subclass private function.  This subclass implementation is still reachable due to polymorphism as long as callers hold pointers to the base type instead of the subclass type.

Why is this ever a good idea?  Because it forces callers who need limited API access to hold pointer types that inherently describe the level of abstraction they need.  In the infobar code, for example, there is something like this:

class ConfirmInfoBarDelegate {

 public:

  virtual void Accept();

};

class AwesomeInfoBarDelegate : public ConfirmInfoBarDelegate {

 public:

  void SomeOtherFunction();

 private:

  virtual void Accept() OVERRIDE;

};

class UserOfAwesomeInfoBars {

 UserOfAwesomeInfoBars() : infobar_(new AwesomeInfoBarDelegate) {}

 void Foo() { infobar_->Accept(); }

 private:

  ConfirmInfoBarDelegate* infobar_;

};

UserOfAwesomeInfoBars needs to instantiate a specific infobar delegate, but its use of the delegate beyond that point is limited to the public APIs provided by a ConfirmInfoBarDelegate.  By making AwesomeInfoBarDelegate::Accept() private, UserOfAwesomeInfoBars' |infobar_| member is forced to reflect the actual level of access the class really needs.  This tells the reader that UserOfAwesomeInfoBars is not going to access SomeOtherFunction(), and forces someone who wants to add this or other deeper dependencies later to at least think about what they're doing first.

PK

[William Chan (陈智昌)]

I highly recommend that people interested in API design in C++ read Scott Meyer's Effective C++.

On Thu, Feb 17, 2011 at 7:56 AM, Ben Goodger (Google) <b...@chromium.org> wrote:

I've been working through a revamp of the Views API (more on that later), and here are a few general thoughts I've had as I've scanned through uses of it in the codebase:

- The API is too big, there are too many vaguely (but not quite) identical ways of doing similar seeming things

- There is little control over the API upward (protected:) in the class hierarchy as well as outward (public:)

- Roles for specific types aren't clearly defined enough.

- There are some functions that are dangerous, misleading or just silly, and they're exposed on the public API and are then [mis]used by people who aren't aware of the real reason for their existence!

- Repetition is GOOD.

I'm trying to solve some of this. Specifically the lessons learned are:

It's important to think of how and why someone would subclass your class, when determining what is public, protected and private. The default should be for the least visibility.

I don't think the paragraph below describes it, but note that anything virtual is part of the interface for subclasses. And note the difference between virtual protected member functions and virtual private member functions. Subclasses can invoke the base class's protected member functions, but not the private ones. So a virtual private member function allows the base class to fully control when a function is called.

Note that sometimes a subclass will want to call the base class's implementation. It is simpler to use implementation inheritance and just make the parent's implementation protected to allow the subtype to call it. But be aware of the pitfalls of implementation inheritance which have been written about extensively on the internet (often leads to the http://en.wikipedia.org/wiki/Fragile_base_class problem) and is discouraged in the Google style guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheritance#Inheritance). Please consider using composition instead, especially if the inheritance hierarchy is nontrivial.

For example, far too many methods on View are public, even though there is no reason for anyone to call them outside of the framework. Event handlers are a good example. I will be making all of these protected. Processing functions that exist only in the framework should be private. There are cases where people override some of these framework processing functions. In pretty much every case the people doing this have not understood the underlying framework and actually been doing something wrong/buggy.

I think it is preferable to have friendships or private interfaces with internal:: types than to make methods that only internal:: types call public and pollute the standard API.

For example, RootView is a type that not everyone needs to know about. It should be an implementation detail of the view hierarchy. Mostly, people just want to communicate with the containing Widget. RootView thus moves to the internal namespace, and knows how to communicate with the Views privately.

It is important to note the intended clients of particular functionality. It is best to assume that functionality is not generic if only one client needs it, especially if the implementation of that functionality is complex.

For example, View contains some complex logic intended to support clipping of native child views to the visible bounds of their ancestor views. The only legitimate client for this functionality I've found is NativeViewHost, another class in the Views framework. I am currently working to ensure it is the only one that can use this functionality.

It is a good idea for classes to do one thing and do it well.

RootView is currently the only cross platform per-Widget object. As a result it has a lot of routines on it that aren't really related to event propagation... they're just there because it was simpler to put them there than duplicate code across the various Widget implementations. In my redesign, RootView is focused on the internal details of event propagation, and there is a new cross platform Widget class that handles the rest.

Repetition, Repetition, Repetition! Do I need to say it again?

If you feel like you're off the beaten track when you're writing code, you're probably doing something wrong, or the API isn't well developed enough and you should try and fix it.

For example, in Views there are established patterns for View ownership, setup, hierarchy, etc. We have tried to make it really easy to achieve a certain set of functionality. We have not always made it easy enough. As one example, it's still too hard to port, and as another there are still some oddities with native widget focus that seem to leave us littered with hacks all over the place. It is worth understanding the underlying problem, what the framework is doing, and attempting to fix the framework generically than bandaiding in an increasing number of places.

When your code looks like other code, it's likely to raise fewer eyebrows and be easier to review.

-Ben

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Paweł Hajdan, Jr.]

On Thu, Feb 17, 2011 at 21:20, William Chan (陈智昌) <will...@chromium.org> wrote:

I highly recommend that people interested in API design in C++ read Scott Meyer's Effective C++.

For people interested in API design I also recommend excellent Joshua Bloch's How to Design a Good API & Why it Matters:

http://www.infoq.com/presentations/effective-api-design

I also found some slides at http://aarontgrogg.com/wp-content/uploads/2009/09/How-to-Build-API-and-why-it-matters.pdf (but it's really worth it to watch the video, or even live presentation if you have a chance).

[Sandy Fordross]

Scot Meyers' book is an excellent general read for C++. There is also
a new book called "API Design for C++" by Martin Reddy that has just
been released this year. It covers a ton of material on how to develop
professional and long-lasting interfaces in C and C++. Well worth
checking out if you're doing any kind API design work. See:

http://APIBook.com/

Sandy.