3 ms·

Feedback is welcome! I wrote this library for my own use, but I'm happy to hear any suggestions, code improvements, etc

wereHamster 9y ago
Submit it to https://www.reddit.com/r/haskell/ https://www.reddit.com/r/haskell/ if you want feedback to your code. I doubt most people on HN are fluent enough in Haskell to give you meaningful feedback.
- aviaviavi 9y ago
  Already done, mentioned that here too just in case :)
arthurcolle 9y ago
Title might confuse the audience since this doesn’t do any pricing per se, and only wraps a public API
- amrrs 9y ago
  Self Plug: wrote a similar one for R as an R package https://github.com/amrrs/coinmarketcapr https://github.com/amrrs/coinmarketcapr
endgame 9y ago
* Consider using `Text` instead of `String`, as `String` is a linked-list of `Char` and quite inefficient * Consider refining some of your fields into more constrained types. Sort orders probably only have a couple of valid values, and might reduce to an enumeration instead of `String`. * In `instance ToQueryString PriceRequest`, I'd be surprised if those type annotations are necessary. The inferencer should be able to work out that `req` is a `PriceRequest`. * In `instance FromJSON AggregatedSnapshot`, I think the `read`s should go. I think you want to use the `Float` instance of `FromJSON` instead. (What the `read <$> ...` is doing is parsing to a string, and then using the partial (!) `read` function on the result. JSON responses that have a value that fails to read for that key will crash the program.) * In `instance FromJSON AggregatedSnapshot`, consider `withObject` here too, which encapsulates the pattern you're using.
- thinkpad20 9y ago
  I don't have it in front of me but I would assume the FromJSON instance for Float expects a JSON number, not a string. If the remote API makes the (puzzling) decision to put numeric values in strings, then parsing those will be necessary. Although, I agree that the 'read' function is the wrong one to use here, due to it making the function partial (consider 'readMaybe' instead?)
  - aviaviavi 9y ago
    It does indeed do that, there are numeric values returned as strings (i have _no_ idea why they do that), which is why I had `read` in there. I didnt know about readMaybe though, I'll definitely do that, thanks!
- aviaviavi 9y ago
  Thank you for all of these suggestions, very appreciated! I'll work through implementing these. Also, please see my explanation of the `read` calls below, but I plan to change those to `readMaybe`s, since there is a need to parse numbers from strings ( for whatever reason :/ )
  - endgame 9y ago
    What a bizarre API. You might get some mileage out of writing a helper function like `readParser :: Read a => String -> Parser a` which can call `readMaybe` and inspect the result.

Submit it to https://www.reddit.com/r/haskell/ https://www.reddit.com/r/haskell/ if you want feedback to your code. I doubt most people on HN are fluent enough in Haskell to give you meaningful feedback.

aviaviavi 9y ago
Already done, mentioned that here too just in case :)

arthurcolle 9y ago

Title might confuse the audience since this doesn’t do any pricing per se, and only wraps a public API

amrrs 9y ago
Self Plug: wrote a similar one for R as an R package https://github.com/amrrs/coinmarketcapr https://github.com/amrrs/coinmarketcapr

endgame 9y ago

* Consider using `Text` instead of `String`, as `String` is a linked-list of `Char` and quite inefficient * Consider refining some of your fields into more constrained types. Sort orders probably only have a couple of valid values, and might reduce to an enumeration instead of `String`. * In `instance ToQueryString PriceRequest`, I'd be surprised if those type annotations are necessary. The inferencer should be able to work out that `req` is a `PriceRequest`. * In `instance FromJSON AggregatedSnapshot`, I think the `read`s should go. I think you want to use the `Float` instance of `FromJSON` instead. (What the `read <$> ...` is doing is parsing to a string, and then using the partial (!) `read` function on the result. JSON responses that have a value that fails to read for that key will crash the program.) * In `instance FromJSON AggregatedSnapshot`, consider `withObject` here too, which encapsulates the pattern you're using.

thinkpad20 9y ago
I don't have it in front of me but I would assume the FromJSON instance for Float expects a JSON number, not a string. If the remote API makes the (puzzling) decision to put numeric values in strings, then parsing those will be necessary. Although, I agree that the 'read' function is the wrong one to use here, due to it making the function partial (consider 'readMaybe' instead?)
- aviaviavi 9y ago
  It does indeed do that, there are numeric values returned as strings (i have _no_ idea why they do that), which is why I had `read` in there. I didnt know about readMaybe though, I'll definitely do that, thanks!
aviaviavi 9y ago
Thank you for all of these suggestions, very appreciated! I'll work through implementing these. Also, please see my explanation of the `read` calls below, but I plan to change those to `readMaybe`s, since there is a need to parse numbers from strings ( for whatever reason :/ )
- endgame 9y ago
  What a bizarre API. You might get some mileage out of writing a helper function like `readParser :: Read a => String -> Parser a` which can call `readMaybe` and inspect the result.