Changes between Version 41 and Version 42 of MapGuideCodingStandards

Timestamp:

Sep 19, 2022, 5:21:06 AM (2 years ago)

Author:

jng

Comment:

More public API design considerations around overloads and default values

MapGuideCodingStandards

@@ -168 +168 @@
 Do not bother/try making public API methods `const`-correct, it will not affect SWIG interface generation and may even complicate or break it in some cases.
 
+Do not express default values for any parameters in the method signature, SWIG may not reliably map such a concept for our 3 language targets, assume the caller needs to pass all the values for any given method
+
 Anything not on the above list is something assumed to not be reliably SWIG-wrappable with consistent behavior across our 3 managed language targets and should not be used.
 
@@ -174 +176 @@
  * `finalize` is a bad name for a new public API method because this is the name of the built-in method in Java classes that will be called when an object is about to be garbage collected
 
+Avoid having too many parameters in your new public API method. If you are starting to have too many parameters in your method, this is a sign to introduce a new "options" class for that method instead.
+
 For private/internal methods, you are free to name and define method parameters and return type in any way you see fit for best C++ runtime performance.
+
+=== A remark about overloaded methods ===
+
+Avoid introducing overloaded methods or methods that may have overloaded variants in the future to the public API surface if possible. Using an "options" parameter class pattern can future-proof the need to introduce new overloads down the road as you can just introduce new methods on the "options" class itself and update your implementation to handle the new option class methods accordingly.
+
+If you have to use overloaded methods, do not introduce an overload to a virtual method whose new signature only exists on the derived class and not its base. This makes such a class difficult to wrap for our PHP language target and requires a language-specific SWIG `%rename` workaround.
+
+An example of where such a scheme made PHP class generation difficult is in the `GetDistance` method of `MgCoordinateSystemMeasure`. It's parent `MgMeasure` class defines `GetDistance` with this signature:
+
+{{{
+class MgMeasure
+{
+public:
+    ...
+    virtual double GetDistance(MgCoordinate* coord1, MgCoordinate* coord2) = 0;
+};
+}}}
+
+In `MgCoordinateSystemMeasure`, this `GetDistance` method exists in 2 overloaded forms
+
+{{{
+public:
+    ...
+    virtual double GetDistance(MgCoordinate* coord1, MgCoordinate* coord2) = 0;
+    virtual double GetDistance(double x1, double y1, double x2, double y2)=0; //New overload only in MgCoordinateSystemMeasure
+}}}
+
+When generated by SWIG as-is, the .net and Java proxy classes are correct, but the PHP proxy class for `MgCoordinateSystemMeasure` will cause the PHP binding to throw a PHP fatal error of the form:
+
+```
+PHP Fatal error:  Declaration of MgCoordinateSystemMeasure::GetDistance(MgCoordinate|float|null $arg1, MgCoordinate|float|null $arg2, float $arg3, float $arg4): float must be compatible with MgMeasure::GetDistance(?MgCoordinate $arg1, ?MgCoordinate $arg2): float in Unknown on line 0
+```
+
+The remedy was to add a SWIG `%rename` directive to the PHP language binding configuration for this one particular method signature to rename the method to `GetDistanceSimple` avoiding the resulting method signature conflict error above.
+
+By avoiding introducing overloaded methods and preferring the use of an "options" class instead, we avoid this PHP-specific problem and ensure consistent SWIG wrapper generation across our supported language targets.
 
 == Object Creation and Runtime Speed ==