> On Sept. 20, 2016, 12:06 p.m., Aleix Pol Gonzalez wrote:
> > KTp/Declarative/messages-model.cpp, line 222
> > <https://git.reviewboard.kde.org/r/128954/diff/1/?file=477161#file477161line222>
> >
> >     Use iterators?
> 
> Alexandr Akulich wrote:
>     How to get an index from iterator?
> 
> Aleix Pol Gonzalez wrote:
>     You can count in parallel without calling `QList::operator[]` on the 
> index.
> 
> Alexandr Akulich wrote:
>     Actually I call ```const T &at(int i) const;```. And what is the reason 
> to use iterators, if you still suggest to use a counter? Do you actually 
> think that
>     ```
>         int i = d->messages.count() - 1;
>         for (auto it = d->messages.constEnd(); it != 
> d->messages.constBegin(); --it, --i) {
>             if (sentTimestamp > it->message.time()) {
>                 newMessageIndex = i;
>                 break;
>             }
>             // Or do you suggest to place --i; here?
>         }
>     ```
>     is more readable, than plain
>     ```
>         for (int i = d->messages.count() - 1; i >= 0; --i) {
>             if (sentTimestamp > d->messages.at(i).message.time()) {
>                 newMessageIndex = i;
>                 break;
>             }
>         }
>     ```
>     ?
>     
>     IMO it is a common practice to use iterators if you don't need index (and 
> just use the index otherwise). It is a bit more optimal and I don't made an 
> error in the condition.
> 
> Aleix Pol Gonzalez wrote:
>     You can use reverse iterators:
>     http://doc.qt.io/qt-5/qvector.html#crbegin
> 
> Alexandr Akulich wrote:
>     I know about the reverse methods and it is not any better without some 
> reverse adaptor in QList.
>     
>         int i = d->messages.count() - 1;
>         for (auto it = d->messages.crbegin(); it != d->messages.crend(); 
> ++it, --i) {
>             if (sentTimestamp > it->message.time()) {
>                 newMessageIndex = i;
>                 break;
>             }
>             // Or do you suggest to place --i; here?
>         }
>         
>     Why it is better? It reads worse and it looks like "iterators for the 
> iterators" without a real benefits. It does not save us from an error, 
> because we need the (reverse) counter. Do we have a coding convention for 
> this case?

I'd say in general the coding convention is to use iterators whenever possible, 
since we can ensure the access to the random object will be optimal. If you 
really feel like this is worse, feel free to use something else. I didn't come 
here to play sargeant.


- Aleix


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128954/#review99301
-----------------------------------------------------------


On Sept. 20, 2016, 11:51 a.m., Alexandr Akulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128954/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2016, 11:51 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> -------
> 
> Implemented message sort by sent timestamp (if available).
> 
> This fixes order of scrollback messages.
> 
> 
> Diffs
> -----
> 
>   KTp/Declarative/messages-model.cpp dc1088c 
> 
> Diff: https://git.reviewboard.kde.org/r/128954/diff/
> 
> 
> Testing
> -------
> 
> Works, fixes the issue.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Reply via email to