On 26.05.2013 05:27, Friedrich Brunzema wrote:
> All,
>
> I'm playing around in the code - was wondering if the developer
> community likes the attached refactoring. I'm a bit scared, because
> the change (in terms of LOC) is fairly big and there are no unit
> tests to make sure I have not broken anything. Functionality-wise
> everything is still the same. Stefan caught a small bug in my last
> refactoring effort - thanks!
>
> The idea is to make the method shorter and easier to understand
> (abstraction, information hiding)- I'm introducing a small data class
> to hold the local variables that are needed by the menu
> functionality.
>
> I have only done minimal testing, but have found nothing amiss so
> far. To be thorough every context menu would need to be tested - or
> one could do a thorough code review.
>
> I don't want to check this in until I hear from Stefan and/or others.
> I can do this type of refactoring also for the other context menu
> code - in that case some renaming of methods would be required. I
> will only make this effort if people agree that this makes the code
> better, otherwise there is no point.
Looks good. And everything seems to work fine.
a few comments though:
+void CLogDlg::AdjustContextMenuAnchorPointIfKeyboardInvoked( CPoint
&point, int selIndex )
I would extend this method to take a pointer to a list control as well.
Then you can use that same method also for the context menu on the
changed files list later.
+ ContextMenuInfo* pCmi = new ContextMenuInfo();
I don't like using 'new' if it can be avoided or if it's necessary, you
should use std::unique_ptr<> or std::shared_ptr<> to hold the memory.
So you should either change the GetContextMenuInfo() to
GetContextMenuInfo(ContextMenuInfo& Cmi) and fill the data on the object
passed to that method (and the object allocated on the stack), or store
the returned pointer of this method in an std::unique_ptr.
case ID_OPENWITH:
bOpenWith = true;
- // fallthrough
+ // fall-through
case ID_OPEN:
Since you've created a method for opening, remove the fall-through and
just call that function for each of these case labels. That way you can
get rid of the bOpenWith flag.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest interface to (Sub)version control
/_/ \_\ http://tortoisesvn.net
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3056446
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].
Received on 2013-05-26 09:51:16 CEST