Review Request: Review API of the new Path class

[Andreas Pakulat]

> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 311
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line311>
> >
> >     Hmm, I think I'd separate the remote-url part from the path itself here. There are some places in the code where you have explicit checks for remote-urls and then take the first or second item from the list. Having a separate member would make that code a lot clearer.
> 
> Milian Wolff wrote:
>     I also thought about that but deliberately decided against it for space reasons (saving one pointer for local paths). But I agree that it might be a bit over-optimized. I'll revisit the code and take another look at it.
> 
> Andreas Pakulat wrote:
>     I see, but in this case I'd say optimization comes after code-readability. So once everythings ported we should measure the difference between saving the pointer or not and then decide wether saving it outweighs the special code in the various functions - at least IMO.
> 
> Milian Wolff wrote:
>     I did compare it back then but sadly I just realize that my results in the commits are not comparable: 949ed44e369d8d3ce798737f1a3517bc9fe48caa vs c6dca99c710f99c7ddbc47b00dd70157f7d3a076 also shows different numbers for the other datatypes... I'll redo the measurement and figure out whether it's worth it.
> 
> Milian Wolff wrote:
>     Looking at it again, I have to say that the code readability would not increase considerably imo... Especially since you'd now have to compare/check two variables instead of just one the code becomes bigger which is arguably also worse to read.
>     
>     And so far only 6 places or so need to take special care when dealing with the aggregated prefix-in-m_data. Imo this is not so bad and I'd like to keep it as-is.

So you think that this (from setFilename):

    if( m_data.isEmpty() ) {
      m_data.append(name);
    } else {
      m_data.last(name);
    }

is not easier to understand than the current code:

    if (m_data.isEmpty() || (!isLocalFile() && m_data.size() == 1)) {
        // append the name to empty Paths or remote Paths only containing the Path prefix
        m_data.append(name);
    } else {
        // overwrite the last data member
        m_data.last() = name;
    }

I wholeheartedly disagree. Or this one (from up/parent):

    if( m_data.size() == 1 ) {
      ret.m_data.last().clear();
    } else {
      ret.m_data.pop_back();
    }

is certainly easier to understand than this one (and both are actually shorter as well):

    if (m_data.size() == (1 + (isRemote() ? 1 : 0))) {
        // keep the root item, but clear it, otherwise we'd make the path invalid
        // or a URL a local path
        ret.m_data.last().clear();
    } else {
        ret.m_data.pop_back();
    }

The reason is simply that its more self-explanatory to someone who hasn't written up the code. In the current code one first has to think through why with isRemote() it compares to 2 before clearing the last element, so you always have to keep that special behaviour in the back of your head when reading or debugging the code. Can you show the example where you actually get more code for the separation of url and path (yeah I'm too lazy to look through the complete implementation)?


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108196/#review24736
-----------------------------------------------------------


On Jan. 5, 2013, 10:59 a.m., Andreas Pakulat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108196/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 10:59 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Discuss API of the new Path class
> 
> 
> Diffs
> -----
> 
>   project/path.h PRE-CREATION 
>   project/path.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108196/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andreas Pakulat
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130107/98bebe20/attachment.html>