Code Review, puhleeze
-
Normally, I'm pretty easy going with other people's code, but I tripped across this gem :vegemite: in some MFC code for the CDateTimeCtrl class. The method will never return FALSE and even goes so far as to ensure that under two conditions, the return value is set to TRUE, even though it has been initialised to TRUE. Go figure.
BOOL CDateTimeCtrl::GetTime(COleDateTime& timeDest) const
{
SYSTEMTIME sysTime;
BOOL bRetVal = TRUE;LRESULT result = ::SendMessage(m\_hWnd, DTM\_GETSYSTEMTIME, 0, (LPARAM) &sysTime); if (result == GDT\_VALID) { timeDest = COleDateTime(sysTime); bRetVal = TRUE; ASSERT(timeDest.GetStatus() == COleDateTime::valid); } else if (result == GDT\_NONE) { timeDest.SetStatus(COleDateTime::null); bRetVal = TRUE; } else timeDest.SetStatus(COleDateTime::invalid); return bRetVal;}
Even better the DTM_GETSYSTEMTIME message only ever returns GDT_VALID or GDT_NONE. Or at least that is what the docs say. Interestingly the CTime 'equivalent' method, returns the response to the message. I suspect that the intention was to check the COleDateTime object's status in order to determine what happened in the GetTime call. But if that's the case why make the method BOOL. :confused: :confused: :confused: :confused:
-
Normally, I'm pretty easy going with other people's code, but I tripped across this gem :vegemite: in some MFC code for the CDateTimeCtrl class. The method will never return FALSE and even goes so far as to ensure that under two conditions, the return value is set to TRUE, even though it has been initialised to TRUE. Go figure.
BOOL CDateTimeCtrl::GetTime(COleDateTime& timeDest) const
{
SYSTEMTIME sysTime;
BOOL bRetVal = TRUE;LRESULT result = ::SendMessage(m\_hWnd, DTM\_GETSYSTEMTIME, 0, (LPARAM) &sysTime); if (result == GDT\_VALID) { timeDest = COleDateTime(sysTime); bRetVal = TRUE; ASSERT(timeDest.GetStatus() == COleDateTime::valid); } else if (result == GDT\_NONE) { timeDest.SetStatus(COleDateTime::null); bRetVal = TRUE; } else timeDest.SetStatus(COleDateTime::invalid); return bRetVal;}
Even better the DTM_GETSYSTEMTIME message only ever returns GDT_VALID or GDT_NONE. Or at least that is what the docs say. Interestingly the CTime 'equivalent' method, returns the response to the message. I suspect that the intention was to check the COleDateTime object's status in order to determine what happened in the GetTime call. But if that's the case why make the method BOOL. :confused: :confused: :confused: :confused:
Did this start in the Rant and Rave forum or was it moved due to it's politically incorrect nature? I believe that this post should have been posted in the Lounge. Traffic is higher and you would have received useful feedback. After all you were just pointing out some pathetic code that ships with MFC. Michael Martin Pegasystems Pty Ltd Australia martm@pegasystems.com +61 413-004-018 "Don't belong. Never join. Think for yourself. Peace" - Victor Stone
-
Did this start in the Rant and Rave forum or was it moved due to it's politically incorrect nature? I believe that this post should have been posted in the Lounge. Traffic is higher and you would have received useful feedback. After all you were just pointing out some pathetic code that ships with MFC. Michael Martin Pegasystems Pty Ltd Australia martm@pegasystems.com +61 413-004-018 "Don't belong. Never join. Think for yourself. Peace" - Victor Stone
politically incorrect nature LOL :-D Chris