Make findComponent() private?

[tkuchida]

I think it's confusing to present users with both getComponent() and findComponent(). These methods can be used (almost) interchangeably if the path is known ("almost" because one returns a reference and the other returns a pointer). I think it would be preferable to have a single getComponent() method that calls findComponent() if necessary.

[chrisdembia]

I recently advocated for making it public (I thought it was protected or private). I would like two public methods:

• getComponent() must be given either a valid relative or absolute path. If there is no component at that path, it throws an exception.
• findComponent() takes the name (not a path) of a component, and searches recursively downward for a match, and returns nullptr if no component is found.

I think there are good use cases for both of these public methods.

[tkuchida]

Currently, the doxygen for getComponent() and findComponent() indicates that both methods can take a path, so it seems that either method could be used if the full path is known. Having a single method would ensure the fast "get" is used whenever possible, and the slower "find" is used only when necessary. A suggestion to include the full path name could be printed when "find" is called.

I would expect a user to try getComponent() first and, if an exception is thrown, to just switch to findComponent() (if they know it exists) and then change all references to pointers.

[chrisdembia]

If I want to check if a component "foo" is in a model, I can do either:

try {
   getComponent("foo");
} catch (...) {
    ...
}

or

auto* foo = findComponent("foo");
if (foo != nullptr) {
    ...
}

The latter is cleaner.

Having a single method would ensure the fast "get" is used whenever possible, and the slower "find" is used only when necessary.

I think this is more how @aseth1 had it originally and I was opposed to it. You want one method that gets the component you asked for (unambiguously), and a separate method that will search for a component. Those are different requests, and so I think it is cleaner to have separate methods.

It'd be better to chat in person I think.

[tkuchida]

You want one method that gets the component you asked for (unambiguously), and a separate method that will search for a component.

I think this clarifies our disagreement. IMO, the user won't care what OpenSim needs to do behind the scenes to get the Component. Just doing the search and gently suggesting that a direct "get" would be faster (and perhaps even printing out the full path for the user) would be much friendlier than throwing an exception and telling the user the Component doesn't exist on the specified path. We already have "get" and "upd" prefixes to deal with; is it necessary to add a "find" into the mix? The cognitive load is already high with templatized methods, "Connector" vs. "Connectee", etc.

If the concern is about determining whether a Component exists, it seems like a Model::hasComponent() method would be appropriate, or getComponent() could return a null… perhaps something like this:

class OSIMSIMULATION_API Model : public ModelComponent {
...
public:
    static Component& NOT_FOUND; //if this sort of thing would work...
...

Happy to discuss in more detail.

[chrisdembia]

I'm tempted to respond but I would prefer to talk in person. I do think that a hasComponent() is still necessary.

[chrisdembia]

In discussion with @aseth1 and @tkuchida:

1. Make findComponent() private.
2. Create a new method that does not return a component, but prints out the paths of components matching a given substring.
3. Potentially create a new ComponentListFilter that checks if a component's path matches a substring.

[chrisdembia]

Fixed by #1030.