=BG= more robust way of handling optional parameters, including graceful...

[bguiz]

=BG= more robust way of handling optional parameters, including gracefully degradation for backward compatibility, for the directions API

[bguiz]

@moshen I have done this for just the one API that I was working with.

However, I am aware that the other APIs too have optional parameters, and the same technique can be applied to them. If you like this, then I can genericise it, and refactor the entire class.

Let me know your you thoughts on this, especially w.r.t. to my means of preserving backward compatibility.

[bguiz]

FYI, the itch that needed to be scratched was:

"mode (defaults to driving) — Specifies the mode of transport to use when calculating directions. Valid values are specified in Travel Modes. If you set the mode to "transit" you must also specify either a departure_time or an arrival_time." here

So when I set mode to transit, I needed to be able to set arrival_time or departure_time as well.

[moshen]

@bguiz : thanks for this. I had been playing around with this type of thing in my private branch. I can definitely understand the 'itch' you were scratching. The api has changed somewhat since this lib was first put out there (also just a list of args was probably a poor choice in the first place).

I'll take a look, can't promise I or another maintainer will merge, but it's certainly a direction we would like to go with the lib.

[bguiz]

@moshen I have another idea that is more of a complete refactor - and would break backward compatibility - that I think would make this library more robust in the way it deals with API parameters. I'd be keen to submit another pull request for that; before I do so however, I'd like to ask the maintainers of this project, yourself included, how much of a priority maintaining backward compatibility is? That would determine what approach I adopt for this refactor.

[moshen]

@regality , @grobot ~ Any comments?

[dcromer]

I used this package a bit and had some feedback that I believe fits in this issue:

1) The optional parameters COULD be handled better, as noted in this issue. 2) The method signatures are long and unwieldy.

For example, take:

exports.distance = function(origins, destinations, callback, sensor, mode, alternatives, avoid, units, language) ...

This could be written in an easier-to-handle format like:

exports.distance = function(options, callback)...

The defaults and required params can be done in a few ways.

1) underscorejs's defaults for defaulting undefined params: (http://underscorejs.org/#defaults) 2) node-validator for required & other type-specific validation (https://github.com/chriso/node-validator). This might provide the user-friendly feedback you're looking for.

Either way, a nice lib. Thanks for your work on it.

[bguiz]

@dcromer Yup, using an options hash was precisely what I had in mind, should preserving backward compatibility not be an issue. Still waiting on a reply from @moshen and other maintainers of this lib for a reply. Notwithstanding, I might maintain my own fork. Thanks for the suggestions on validation and defaults!

On 14 October 2013 15:14, dcromer notifications@github.com wrote:

I used this package a bit and had some feedback that I believe fits in this issue:

1) The optional parameters COULD be handled better, as noted in this issue. 2) The method signatures are long and unwieldy.

For example, take:

exports.distance = function(origins, destinations, callback, sensor, mode, alternatives, avoid, units, language) ...

This could be written in an easier-to-handle format like:

exports.distance = function(options, callback)...

The defaults and required params can be done in a few ways.

1) underscorejs's defaults for defaulting undefined params: ( http://underscorejs.org/#defaults) 2) node-validator for required & other type-specific validation ( https://github.com/chriso/node-validator). This might provide the user-friendly feedback you're looking for.

Either way, a nice lib. Thanks for your work on it.

— Reply to this email directly or view it on GitHubhttps://github.com/moshen/node-googlemaps/pull/24#issuecomment-26234797 .

[regality]

I set up a proof of concept for a refactor.

The two files of interest are https://github.com/ifit/node-googlemaps/blob/two-point-oh/lib/core.js, which is the guts of the refactor, and https://github.com/ifit/node-googlemaps/blob/two-point-oh/lib/index.js which is the implementation. This will allow us to have a clean standard way of defining each method.

It is also setup to be self documenting. The last two lines in index.js output this:

googlemaps.geocode:
  documentation: http://code.google.com/apis/maps/documentation/geocoding/
  required: address
  optional: bounds, region, language
  path: /maps/api/geocode/json
googlemaps.places:
  required: location, key
  optional: types, lang, name, radius
  defaults: rankby:prominence
  path: /maps/api/place/search/json

The method method, used in lib/index.js, takes exports as the first argument, setup('name') as the second, and then as many functions as you want to pass in. Some methods, such as geocode, have no unique logic. They can be defined with required and optional parameters and the request to be made. This should make refactoring relatively easy.

The places method required some custom validation, so we just insert a function in the middle to handle that.

This would be a "2.0" kind of thing and break all backwards compatibility, but I think it would be a good way to approach this library.

Thoughts?

[fabriziomoscon]

Please see #70