gferrin / bitfinex

node.js wrapper for bitfinex cryptocurrency exchange
25 stars 20 forks source link

orderbook without parameters invokes API call with limit_bids and limit_asks #13

Closed dutu closed 9 years ago

dutu commented 9 years ago

when orderbook method is called without the optional parameters, the native API is called with limit_bids=50&limit_asks=50. https://github.com/gferrin/bitfinex/blob/master/src/bitfinex.coffee#L103

gferrin commented 9 years ago

These are the defaults, now they accept optional parameters.

dutu commented 9 years ago

same as commented on #12. perhaps you can just include these parameters if they are specified, else call the native API without parameter and the bitfinex will respond with the defaults with whatever default values are. With this suggestion you can also avoid updating your wrapper code in case bitfinex changes the default value.

gferrin commented 9 years ago

Yeah good point. Here's what the function looks like now:

orderbook: (symbol, options, cb) ->

        allowed_options = ['limit_bids', 'limit_asks', 'group']
        query_string = '/?'
        index = 0
        uri = 'book/' + symbol 

        if typeof options is 'function'
            cb = options
        else 
            try 
                for option, value of options
                    if option in allowed_options
                        if index++ > 1
                            query_string += '&' + option + '=' + value
                        else
                            query_string += option + '=' + value

                if index > 0 
                    uri += query_string
            catch err
                return cb(err)

        @make_public_request(uri, cb)
dutu commented 9 years ago

yes, it's ok, maybe a bit complicated? :). Perhaps as wrapper function it should pass the parameters/options it receives (i.e. no checking/setting default values and not even if the options are allowed or not). Perhaps you could build the query_string by just going through the options keys.

dutu commented 9 years ago

also when there are no options should query_string be "" rather than '/?'?

dutu commented 9 years ago

by not checking allowed_options, you can also avoid code changes in case Bitfinex is changing the possible options for some reason.

gferrin commented 9 years ago

query_string only gets added when index > 0 but I could move it. And yeah that's true about the allowed_options I'll remove the check.

gferrin commented 9 years ago

Here's the new version

orderbook: (symbol, options, cb) ->

        index = 0
        uri = 'book/' + symbol 

        if typeof options is 'function'
            cb = options
        else 
            try 
                for option, value of options
                    if index++ > 1
                        query_string += '&' + option + '=' + value
                    else
                        query_string = '/?' + option + '=' + value

                if index > 0 
                    uri += query_string
            catch err
                return cb(err)

        @make_public_request(uri, cb)
dutu commented 9 years ago

thanks a lot. Really appreciated