[Marble-devel] Review Request 125377: Open Location Code Search

[Dennis Nienhüser]

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


Looks good and works fine, just some style nitpicking.


src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchPlugin.cpp (line 44)
<https://git.reviewboard.kde.org/r/125377/#comment60800>

    Copy-paste error, please adjust



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h (line 30)
<https://git.reviewboard.kde.org/r/125377/#comment60801>

    Please move to the private section. The method can be marked const also.



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h (line 36)
<https://git.reviewboard.kde.org/r/125377/#comment60802>

    Method can be const



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h (line 41)
<https://git.reviewboard.kde.org/r/125377/#comment60803>

    Method can be const



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 22)
<https://git.reviewboard.kde.org/r/125377/#comment60804>

    Please remove and use a local variable in isValidOLC instead:
    
    ```
    // It must have only one SEPARATOR located at an even index in
    // the string.
    QChar const separator('+');
    int separatorPos = olc.indexOf(separator);
    ...
    ```



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 23)
<https://git.reviewboard.kde.org/r/125377/#comment60805>

    Please remove and use a local variable in isValidOLC instead:
    
    ```
    int const separatorPosition = 8;
    // It must be a full open location code.
    ...
    ```



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 24)
<https://git.reviewboard.kde.org/r/125377/#comment60806>

    Please remove and use a local variable in isValidOLC instead:
    
    ```
    // Test the characters before the SEPARATOR.
    QChar const suffixPadding('0');
    ```



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 25)
<https://git.reviewboard.kde.org/r/125377/#comment60807>

    Please remove and use a local variable in the ctor:
    
    ```
    // initialize the charIndex map
    QString const acceptedChars = "23456789CFGHJMPQRVWX";
    for(int index = 0; index < acceptedChars.size(); ++index) {
        charIndex[acceptedChars[index]] = index;
    }
    ```



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 54)
<https://git.reviewboard.kde.org/r/125377/#comment60808>

    GeoDataStyle is now used as a shared ptr, please replace with
    
    ```
    GeoDataStyle::Ptr style = GeoDataStyle::Ptr(new GeoDataStyle());
    ```



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 164)
<https://git.reviewboard.kde.org/r/125377/#comment60809>

    Please use curly brackets {} also for one-liners



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 167)
<https://git.reviewboard.kde.org/r/125377/#comment60810>

    Please use curly brackets {} also for one-liners



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 171)
<https://git.reviewboard.kde.org/r/125377/#comment60811>

    Please use curly brackets {} also for one-liners



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 179)
<https://git.reviewboard.kde.org/r/125377/#comment60814>

    Please use curly brackets {} also for one-liners



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 181)
<https://git.reviewboard.kde.org/r/125377/#comment60812>

    Please use curly brackets {} also for one-liners



src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 183)
<https://git.reviewboard.kde.org/r/125377/#comment60813>

    Please use curly brackets {} also for one-liners


- Dennis Nienhüser


On Sept. 24, 2015, 8:17 p.m., Constantin Mihalache wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125377/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2015, 8:17 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Bugs: 351308
>     http://bugs.kde.org/show_bug.cgi?id=351308
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Implemented the open location code search plugin as described on the bugtracker.
> 
> 
> Diffs
> -----
> 
>   src/plugins/runner/CMakeLists.txt 005f1b6 
>   src/plugins/runner/openlocation-code-search/CMakeLists.txt PRE-CREATION 
>   src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchPlugin.h PRE-CREATION 
>   src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchPlugin.cpp PRE-CREATION 
>   src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h PRE-CREATION 
>   src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125377/diff/
> 
> 
> Testing
> -------
> 
> Tested with the codes found on Google's [repository](https://github.com/google/open-location-code).
> 
> 
> Thanks,
> 
> Constantin Mihalache
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20151122/f96a3648/attachment-0001.html>