Error messages. #25
Error messages. #25
Conversation
src/apis/googleapi.cpp
Outdated
| @@ -47,6 +47,11 @@ QString GoogleAPI::translate(const QString &input, | |||
| QJsonDocument replyJsonDocument = QJsonDocument::fromJson(replyByteArray.data()); | |||
| QJsonObject replyJsonObject = replyJsonDocument.object(); | |||
|
|
|||
| // If error. | |||
| if (replyJsonObject.contains("error")) { | |||
| throw InvalidArgumentException(); | |||
There was a problem hiding this comment.
I'd suggest to pass error into InvalidArgumentException.
src/mainwindow.cpp
Outdated
| try { | ||
| outputString = api->translate(inputString, sourceLanguage, targetLanguage); | ||
| } catch (const InvalidArgumentException &e) { | ||
| showErrorBox("Text cannot be translated. Invalid API key."); |
There was a problem hiding this comment.
Use message added into InvalidArgumentException and add it to showErrorBox as additional details.
Same with other instances of this exception.
There was a problem hiding this comment.
I did a bit in a different way.
| public: | ||
| void raise() const { throw *this; } | ||
|
|
||
| InvalidArgumentException *clone() const { return new InvalidArgumentException(*this); } |
There was a problem hiding this comment.
return new... should be in new line as well as ending bracket }.
There was a problem hiding this comment.
There was a problem hiding this comment.
This is for inline functions, while this one is not an inline function, its a stateful method.
src/apis/googleapi.cpp
Outdated
| std::string errorMessage = errorJsonObject["message"] | ||
| .toString() | ||
| .toStdString(); | ||
| DLOG(INFO) << "Error " + std::to_string(errorCode) + ": " + errorMessage; |
There was a problem hiding this comment.
I'd suggest to use a single exception handler instead of littering code with logs. Also once you start reading the stack trace of InvalidArgumentException, you wont notice this log line. What you'll see will be InvalidArgumentException with no context. Due to this you'll have to look for additional log messages which will require a lot more time in order to get more details on what caused this issue.
Thats why InvalidArgumentException should have a message inside it.
| @@ -8,9 +8,9 @@ | |||
| */ | |||
| class InvalidArgumentException : public QException { | |||
| public: | |||
| void raise() const { throw *this; } | |||
| inline void raise() const { throw *this; } | |||
There was a problem hiding this comment.
Are you sure these should be marked as inline? Why not just use the regular way throw InvalidArgumentException - what essentially is done by using inline?
Seems like a very funky way to throw exceptions.
There was a problem hiding this comment.
"Specifying inline encourages the compiler to do a better job." from http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inline.
Added functionality for handling errors. Solves #13.