Code review comment for lp:~rpadovani/reminders-app/multiple-accounts

Revision history for this message
Michael Zanetti (mzanetti) wrote :

Hey Riccardo,

nice. Congrats to your first real C++ branch :) Obviously there are a few issues to fix. But you know how picky I am. Don't blame yourself for the length of the list. I intentionally explained lots of things.


Hmm... Why do you save the username? That just makes it more complex.

Instead of saving the username into the config, I would just save the token. This way, on startup we can just load the token from the config, set it to EvernoteConnection and not bother dealing with AccountService at all.
Once the token is set, the username will be automatically refreshed inside the UserStore.


Here's a couple of other things I'd like to have changed:

134 CameraHelper helper;
135 view.engine()->rootContext()->setContextProperty("cameraHelper", &helper);
137 + view.engine()->rootContext()->setContextProperty("accountPreference", new AccountPreference());

This works, but please align it to look the same as the CameraHelper. Apart from the visual difference, there is a theoretical issue with your code too: If you create a variable without "new", it will be allocated on the stack and automatically deleted when the function ends. If you do "new" you will allocate the object on the heap and need to delete it yourself. Now, given that the program is removed from the memory when the main() ends, this is really a theoretical issue here and doesn't really hurt us in practice. But I hope you get the idea and see the benefit of keeping things consistent with the existing code.


199 + var accountName = accountPreference.accountName;
200 + if (accountName) {
201 + var i;
202 + for (i = 0; i < accounts.count; i++) {
203 + if (accounts.get(i, "displayName") == accountName) {
204 + accountService.objectHandle = accounts.get(i, "accountServiceHandle");
205 + }
206 + }
207 + }

As said above, this should be more like:

if (accountPreferences.token) {
    EvernoteConnection.token = accountPreferences.token;
} else {
  // Do the AccountService stuff...


372 +void EvernoteConnection::clearToken()
373 +{
374 + if (!EvernoteConnection::instance()->token().isEmpty()) {
375 + setToken(NULL);
376 + emit tokenChanged();
377 + }
378 +}

Heh, you're inside EvernoteConnection, you can directly access m_token instead of going through EvernoteConnection::instance()->token(). Also, never use NULL if a QString parameter is required. And one more: setToken() already does the emit tokenChanged(), you're emitting that twice now.

This function could (and should) be as short as:

void EvernoteConnection::clearToken()
    emit tokenChanged();


void EvernoteConnection::clearToken()

I guess the latter one is the better one.


56 \ No newline at end of file

Make sure C++ code files always end with a newline. While this is again a theoretical issue in our case, it breaks compilation with some compilers, e.g. on Solaris.


283 + text:"Accounts")
284 + iconName: "contacts-app-symbolic"
285 + visible: accounts.count
286 + onTriggered: {
287 + openAccountPage(true);
288 + }
289 + }
290 +
291 + ToolbarButton {

I personally don't think its good to have the button in the toolbar all the time, given that in practice you will never ever use it :)

How about putting it into the HUD? David, Dani, Alan, what's your opinion on this?

review: Needs Fixing

« Back to merge proposal