Code review comment for lp:~carlos-mazieri/ubuntu-filemanager-app/app-devel-pre4

> 1)
>
> Non-virtual destructors on classes that either inherit or are inherited other
> classes with virtual functions, these should be virtual destructors:
> ~Location()
> ~DiskLocation()
> ~LocationsFactory()
>
> Especially important are the Location ones and the classes deriving from it. A
> basic rule-of-thumb is to have a virtual destructor on any class that has
> virtual functions or that derives from a class that has virtual destructors.
> Deviating from this rule-of-thumb is sometimes acceptable but there should be
> heavy reasons for it, and some comment for why it is so would be appreciated.
> Right now I do not see anything in the code that would warrant them not-being
> virtual destructors.
>

> It'd be good to check the code on the whole that there's not more than what I
> noticed in this code-review

DONE. added virtual for destructors. LocationsFactory does not have any virtual method nor any descendent class.

.
>
> 2)
>
> There is const_cast<...> calls. It's a red flag, casting should only be used
> if definitely needed. Either document why it's needed or try to refactor the
> code to get rid of it. For example:
> const Location * LocationsFactory::setNewPath(const QString& uPath)
>
> Why does it need to return const pointer as it's used in such a way that it's
> modified? Why not have it return non-const pointer and get rid of the
> const_casts.

DONE. removed const

>
> 3)
>
> in someplaces when checking pointer there is:
> if (m_somePointer == 0)
> and other times just:
> if (m_somePointer)
>
> Either is fine IMO, but it'd be good to keep the code-base consistent. So use
> one or the other.

I am used to use "if (pointer)" when asking for avalid address and "if (pointer == 0)" when asking if a pointer is NULL, I could use "if (pointer != 0)" when asking for avalid address, but not "if (!pointer)" which looks like the variable is a bool.

>
> 4)
>
> This perplexes me:
>
> void Location::setFromInfoItem(const DirItemInfo *itemInfo)
> 917 +{
> 918 + if (m_info)
> 919 + {
> 920 + delete m_info;
> 921 + }
> 922 + m_info = const_cast<DirItemInfo*> (itemInfo);
> 923 +}
>

DONE. Changed to Location::setInfoItem(DireItemInfo *itemInfo) (const removed)

>
> the name of the function, setFromInfoItem, along with the const parameter to
> me gives an impression that it will make a copy for the class to use. Instead
> it does a cast and uses the pointer directly. Perhaps do not use parameter as
> "const" and rename the function to setInfoItem(DireItemInfo *itemInfo) ? Here
> I may not understand some details, so feel free to correct me.
>
> 5)
>
> Typos:
>
> LocationsFactory::stringAfterSlahes() ->
> LocationsFactory::stringAfterSlashes()
> DiskLocation::stoptExternalFsWatcher() ->
> DiskLocation::stopExternalFsWatcher()

DONE. Changed.