komola / paymill-node

Node.JS wrapper for the Paymill v2 API
MIT License
24 stars 11 forks source link

Add useful error messages #16

Closed Prinzhorn closed 10 years ago

Prinzhorn commented 10 years ago

New issue, since #7 was about returning an instance of Error. This one is about returning useful error messages instead of just "Response code is not ok".

11 did not include better error messages for the case when the status code is not 200, see https://github.com/komola/paymill-node/blob/3b1cb8dd9d716c35c7fd3dc307f43d39afa9a5e7/lib/main.js#L7 and https://github.com/komola/paymill-node/blob/3b1cb8dd9d716c35c7fd3dc307f43d39afa9a5e7/lib/main.js#L21

I will take a look into it. Maybe some of the other paymill libs (PHP, Java etc.) already contain a mapping off error codes -> readable error message we can copy.

Prinzhorn commented 10 years ago

https://github.com/paymill/paymill-php/blob/master/lib/Paymill/Services/ResponseHandler.php#L14

https://github.com/paymill/paymill-php/blob/master/lib/Paymill/Services/ResponseHandler.php#L347

Prinzhorn commented 10 years ago

I will start implementing it exactly this way.

Prinzhorn commented 10 years ago

I'm basically done, also with tests. But I struggle with what hell this PHP code is doing

private function getErrorMessageFromArray($errorArray)
    {
        $errorMessage = array_shift($errorArray);
        if (is_array($errorMessage)) {
            return $this->getErrorMessageFromArray($errorMessage);
        } else {
            return $errorMessage;
        }
    }

If $errorArray is a nested array of strings, then this method will always return the very last array item inside the very last array item (you get the point, the right most item). Why not just use array_pop? It would only traverse the last item of every level. I implement it the same, but maybe we should change this if it makes sense what I just said (and also tell the PHP team).

sebastianhoitz commented 10 years ago

The difference between array_shift and array_pop is, that shift gives you the first item in the array, while pop gives you the last.

Prinzhorn commented 10 years ago

Sure I know. I realized last night that I got that part wrong, because it will exit with the first non-array it finds, and not the last. So I guess I got that JS part right.