Sign in to follow this  

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

This topic is 2813 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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;
	CoTaskMemFree(resultShellItemName);

   return true;
}



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

Share this post


Link to post
Share on other sites
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 this post


Link to post
Share on other sites
Quote:
Original post by darkcrobat
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.

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 this post


Link to post
Share on other sites

  • 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 this post


Link to post
Share on other sites
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 this post


Link to post
Share on other sites

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



Use braces please.


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


Share this post


Link to post
Share on other sites
Quote:
Original post by Decibit

  • 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)?


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 this post


Link to post
Share on other sites
Quote:
Original post by Gage64
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);

Changed it. Thx for suggestion.

Share this post


Link to post
Share on other sites
Quote:
Original post by Jacob Jingle
BTW, 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 this post


Link to post
Share on other sites
Quote:
Original post by Narf the Mouse
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.

No, since they are dependent on each other.

Quote:
Also, it needs comments! :)

The code is clear! Why would comments add value? Comments carry some work overhead and the danger that they are not accuratly updated when the code is, which can lead to misleading comments which is actually harmful.
Only none-trivial comments make sense, unless you're writing a tutorial for newbies.

Share this post


Link to post
Share on other sites
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 this post


Link to post
Share on other sites
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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by Jacob Jingle
A) 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 this post


Link to post
Share on other sites
Quote:
Original post by Decibit
Your 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 this post


Link to post
Share on other sites
Quote:
Original post by Jacob Jingle
I 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 this post


Link to post
Share on other sites
Quote:
Original post by Jacob Jingle
Quote:
Original post by Decibit
Your 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 Jingle
I 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 this post


Link to post
Share on other sites
Quote:
Original post by Jacob Jingle
Quote:
Original post by TheUnbeliever
Just 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 this post


Link to post
Share on other sites

This topic is 2813 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this