# Grade my simple C++ code [How would you make this better?]

## Recommended Posts

Jacob Jingle    226
I've seen the gurus on this group come up with some insane stuff, so I have to ask, how could my simple code below be better?
#include <comdef.h>
#include <shlobj.h>

_COM_SMARTPTR_TYPEDEF(IFileOpenDialog, __uuidof(IFileOpenDialog));
_COM_SMARTPTR_TYPEDEF(IShellItem, __uuidof(IShellItem));

bool FileOpen(HWND window, LPCWSTR captionText, std::wstring &dirFileName)
{
IFileOpenDialogPtr fileOpenDlg(__uuidof(FileOpenDialog));
if(FAILED(fileOpenDlg->SetTitle(captionText)))
return false;

FILEOPENDIALOGOPTIONS fileOpenDlgOptions;
if(FAILED(fileOpenDlg->GetOptions(&fileOpenDlgOptions)))
return false;

fileOpenDlgOptions &= FOS_FILEMUSTEXIST;
fileOpenDlg->SetOptions(fileOpenDlgOptions);

HRESULT ret = fileOpenDlg->Show(window);

if(FAILED(ret))
return ret == HRESULT_FROM_WIN32(ERROR_CANCELLED);

IShellItemPtr resultShellItem;
if(FAILED(fileOpenDlg->GetResult(&resultShellItem)))
return false;

LPWSTR resultShellItemName = NULL;
if(FAILED(resultShellItem->GetDisplayName(SIGDN_FILESYSPATH, &resultShellItemName)))
return false;

dirFileName = resultShellItemName;

return true;
}


[Edited by - Jacob Jingle on April 1, 2010 12:14:03 PM]

##### Share on other sites
darkcrobat    100
Can you give us some more information? I'm not at all qualified to review this kind of stuff, but maybe a summary of what this is trying to do, what kind of advice you're looking for or any concerns with how it's working would be helpful.

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by darkcrobatCan you give us some more information? I'm not at all qualified to review this kind of stuff, but maybe a summary of what this is trying to do, what kind of advice you're looking for or any concerns with how it's working would be helpful.

I'm just looking for alternative approaches, coding styles, things I should think about, etc to this kind of function. Anything anyone has to offer basically.

##### Share on other sites
Decibit    140

• Implementing it with a good widget library (QT 4) would increase the portability

• Avoiding hard-wired constant strings ("Open File") would help to localize your application

• What are other possible errors? Do you want to inform the user if the file exists but is inacessible (no reading permissions)?

##### Share on other sites
Gage64    1235
One minor thing: Instead of this:

if(FAILED(ret))    if(ret == HRESULT_FROM_WIN32(ERROR_CANCELLED))	return true;    else        return false;

I would write:

if(FAILED(ret))    return ret == HRESULT_FROM_WIN32(ERROR_CANCELLED);

##### Share on other sites
Dom_152    476
if(FAILED(ret))		if(ret == HRESULT_FROM_WIN32(ERROR_CANCELLED))			return true;		else		    return false;

if(FAILED(ret)){		if(ret == HRESULT_FROM_WIN32(ERROR_CANCELLED))                {			return true;                }		else                {		    return false;                }}

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by DecibitImplementing it with a good widget library (QT 4) would increase the portabilityAvoiding hard-wired constant strings ("Open File") would help to localize your applicationWhat are other possible errors? Do you want to inform the user if the file exists but is inacessible (no reading permissions)?

A) I really don't care about portability for the lil project I'm working on. QT would be overkill. BTW, doesn't WINE get rid of the need for this?
B) Yeah, I will change the code above to reflect this. In the end I will probably just throw it all into a class....Or not, I dunno.
C) Just let errors from control be all the warnings they get. Work anything else in later.

Thx for suggestions.

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by Gage64One minor thing: Instead of this:if(FAILED(ret)) if(ret == HRESULT_FROM_WIN32(ERROR_CANCELLED)) return true; else return false;I would write:if(FAILED(ret)) return ret == HRESULT_FROM_WIN32(ERROR_CANCELLED);

Changed it. Thx for suggestion.

##### Share on other sites
demonkoryu    980
Quote:
 Original post by Jacob JingleBTW, doesn't WINE get rid of the need for this?

Only in a broad sense ("it works... somewhat"). First, not everything works with Wine. Second, GDI performance is abysmal; this is especially noticable with custom drawn GUIs. Third, properly ported software fits in with its environment, integrates with the system so to speak. Things like drag'n'drop between apps and desktop, filesystem conventions, etc... and many other things don't work or don't feel right with Wine.

##### Share on other sites
You could probably do all your assigns in one bunch and then all your tests in another bunch. It's a style choice and not important, but it may improve readability.

##### Share on other sites
demonkoryu    980
Quote:
 Original post by Narf the MouseYou could probably do all your assigns in one bunch and then all your tests in another bunch. It's a style choice and not important, but it may improve readability.

No, since they are dependent on each other.

Quote:

Only none-trivial comments make sense, unless you're writing a tutorial for newbies.

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by Narf the MouseAlso, it needs comments! :)

Sry, I no longer put comments in with my code. Instead, if needed, I drop my code into a web page and comment like hell there. I didn't think this function warranted that treatment. Does anyone else do this?

[Edited by - Jacob Jingle on April 1, 2010 7:32:38 AM]

##### Share on other sites
szecs    2990
The only comments I have are the commented out blocks of code.
Code should be clear, naming should be self descriptive. Of course I never release my code, so I use them only.

##### Share on other sites
Quote:
Original post by Jacob Jingle
Quote:
 Original post by Narf the MouseAlso, it needs comments! :)

Sry, I no longer put comments in with my code. Instead, if needed, I drop my code into a web page and comment like hell there. I didn't think this function warranted that treatment. Does anyone else do this?

I don't, but comments that start // TODO ... are far more common in my code than any other. Other than that, I have maybe one comment per file, typically - I prefer to refactor and/or rename to make the code readable by itself. The exception to this is where a function's essentially just a piece of maths and a comment that describes what it's doing in words helps understanding.

I certainly agree that this doesn't warrant commenting. I don't know COM and barely touch WinAPI, but it's obvious what each line does.

##### Share on other sites
Antheus    2409
[out] parameters are usually at the end.

bool FileOpen(HWND window, LPCWSTR captionText, std::wstring &dirFileName)

##### Share on other sites
Decibit    140
Quote:
 Original post by Jacob JingleA) I really don't care about portability for the lil project I'm working on. QT would be overkill.

Your lil project would become even less (to code) if use QT or something similar. As far as I know COM is famous for being verbose. The MFC developers tried to handle this. But I still think that QT is in general a better choice than MFC.

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by DecibitYour lil project would become even less (to code) if use QT or something similar.

No, going lean and mean barebones on this one. Very few dependencies(beyond win 7). I'm not even using boost.

Quote:
 Original post by Decibit As far as I know COM is famous for being verbose. The MFC developers tried to handle this. But I still think that QT is in general a better choice than MFC.

Wahahahahaha, isn't everything better than MFC? <at least it was when I used it some time back>

I looked at QT and it's license seems to be restrictive(have to share source code) for anyone not willing to pay.

Personally, I don't mind sharing my code...I just don't like being told I have to.

##### Share on other sites
demonkoryu    980
Quote:
 Original post by Jacob JingleI looked at QT and it's license seems to be restrictive(have to share source code) for anyone not willing to pay.

Qt Opensource uses the LGPL license, which means that you only have to share modifications to or code derived ("copied and modified") from the library itself. And modifying Qt is rarely needed in my experience.

##### Share on other sites
Decibit    140
Quote:
Original post by Jacob Jingle
Quote:
 Original post by DecibitYour lil project would become even less (to code) if use QT or something similar.

No, going lean and mean barebones on this one. Very few dependencies(beyond win 7). I'm not even using boost.

Don't forget the time and effort one must invest in learning QT. You are right, it's not always worth it.

Quote:
 Original post by Jacob JingleI looked at QT and it's license seems to be restrictive(have to share source code) for anyone not willing to pay.

They also distribute it under the LGPL.

##### Share on other sites
Quote:
 Original post by Jacob JingleI'm not even using boost.

Just in case you don't know, the overwhelming majority of the Boost libraries are header-only.

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by TheUnbelieverJust in case you don't know, the overwhelming majority of the Boost libraries are header-only.

Who programs C++ and doesn't know this? [smile]

##### Share on other sites
Quote:
Original post by Jacob Jingle
Quote:
 Original post by TheUnbelieverJust in case you don't know, the overwhelming majority of the Boost libraries are header-only.

Who programs C++ and doesn't know this? [smile]

Beginners, who are those most wont to post in 'For Beginners'. ;-)

##### Share on other sites
Jacob Jingle    226
Quote:
 Original post by TheUnbelieverBeginners, who are those most wont to post in 'For Beginners'. ;-)

Wahahahahahaha, good point, I forgot what group I was in.