Code review comment for lp:~qqworini/ubuntu-rssreader-app/database-bug-fixed

Revision history for this message
Roman Shchekin (mrqtros) wrote :

Joey, sry for late answer!

Look this line

if (params.isAll || params == undefined) // or miss params

second condition will never return true. Swap conditions.
Cycle in function addArticles contains a lot of additional temp variables.
I think we must use minimum of them.
But if performance satisfied you, code is ok :)

And last advice - for back compatibility save function addArticle :) Mb it
will be helpful for us later!

BR,
Roman.

2013/6/19 Joey Chan <email address hidden>

> Good, wait for your further email
>
>
> 2013/6/19 Roman Shchekin <email address hidden>
>
> > Joey, I found some issues in your code, I will explain them later, when
> > I'll have access to PC.
> >
> > But in the same time I found good solutions, like json.stringify and so
> > on. We must merge our implementations of addArticles to get best one.
> >
> >
> > 19.06.13 11:52 Joey Chan написал(а):
> >
> > Joey Chan has proposed merging
> > lp:~qqworini/ubuntu-rssreader-app/database-bug-fixed into
> > lp:ubuntu-rssreader-app.
> >
> >
> > Commit message:
> > bugs fixed, especially add a function "addArticles" to avoid the hard
> > drive performance issue
> >
> >
> > Requested reviews:
> > Ubuntu RSS Feed Reader Developers (ubuntu-rssreader-dev)
> >
> >
> > For more details, see:
> >
> >
> >
> https://code.launchpad.net/~qqworini/ubuntu-rssreader-app/database-bug-fixed/+merge/170257
> >
> >
> > bugs fixed, especially add a function "addArticles" to avoid the hard
> > drive performance issue
> >
> > --
> >
> >
> https://code.launchpad.net/~qqworini/ubuntu-rssreader-app/database-bug-fixed/+merge/170257
> >
> > Your team Ubuntu RSS Feed Reader Developers is requested to review the
> > proposed merge of lp:~qqworini/ubuntu-rssreader-app/database-bug-fixed
> into
> > lp:ubuntu-rssreader-app.
> >
> >
> >
> >
> >
> >
> https://code.launchpad.net/~qqworini/ubuntu-rssreader-app/database-bug-fixed/+merge/170257
> > You are the owner of
> lp:~qqworini/ubuntu-rssreader-app/database-bug-fixed.
> >
>
> --
>
> https://code.launchpad.net/~qqworini/ubuntu-rssreader-app/database-bug-fixed/+merge/170257
> Your team Ubuntu RSS Feed Reader Developers is requested to review the
> proposed merge of lp:~qqworini/ubuntu-rssreader-app/database-bug-fixed into
> lp:ubuntu-rssreader-app.
>

« Back to merge proposal