I could use some Advice on some REALLY Ugly Code

Question

0.00/5 (No votes)

See more:

First off programming is a hobby of mine. I didn't go to school for it and I don't work in the field. Programming is a hobby, the problem solving excites my brain... That being said as a Exercise I decided to write a complex application framework. I figured out how to check the Screen Resolution to determine if it meets the programs minimum resolution requirements and prompt the user to change the resolution if it doesn't. If they choose to change the resolution it opens the control panel... waits for the control panel to close (via checking for its handle) and then rechecking the resolution. It works but it's ugly and I know there HAS to be a better way to do it. So here is that section of code. Any suggestions or opinions would be greatly appreciated. Thanks in advance.

C++

int AppClass::InitializeProgram(void){
	while( ScreenResolution.x < MinScreenResolution.x || 
		ScreenResolution.y < MinScreenResolution.y){
		std::wstring Buffer = MyApp->SetText(SCREEN_RESOLUTION) +
								std::to_wstring(MinScreenResolution.x) +
								TEXT("X") +
								std::to_wstring(MinScreenResolution.y) +
								TEXT("\n") +
								MyApp->SetText(SCREEN_RESOLUTION_PROMPT);

		std::wstring Caption = MyApp->SetText(ERROR_CAPTION);
		if (MessageBox(NULL, Buffer.c_str(), Caption.c_str(), MB_YESNO | MB_ICONSTOP) == IDNO){
			return SCREEN_RESOLUTION;
		}

		HRESULT hr = S_OK;

		hr = CoInitializeEx(NULL, COINIT_MULTITHREADED);
		if (SUCCEEDED(hr)){
			IOpenControlPanel *pocp;

			hr = CoCreateInstance(CLSID_OpenControlPanel, NULL, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&pocp));
			if (SUCCEEDED(hr)){
				hr = pocp->Open(TEXT("Microsoft.Display"), TEXT("Settings"), NULL);
			}
			pocp->Release();
		}
		HWND DisplayPanel = FindWindowEx(NULL, NULL, TEXT("CabinetWClass"), TEXT("Screen Resolution"));
		if (DisplayPanel = NULL){
			MessageBox(NULL, TEXT("DIDNT WORK"), TEXT("Caption"), MB_OK | MB_ICONEXCLAMATION);
		}
		while (FindWindowEx(NULL, NULL, TEXT("CabinetWClass"), TEXT("Screen Resolution")) != NULL){
			Sleep(200);
		}

		MyApp->QueryScreenResolution();
	}

Posted 22-Jan-15 21:48pm

drkwlf

Add a Solution

Comments

Richard MacCutchan 23-Jan-15 3:59am

Rather than asking the user to change the screen resolution (why would he/she want to?), your program should be self adjusting to take account of what the screen resolution is set to.

drkwlf 23-Jan-15 4:08am

I agree completely, to a point. I learned by using my netbook a lot of programs simply cannot be used on their low resolution screens. So by having the program setting a minimum say 800 x 600, the application will always work. I read somewhere that you should design for the lowest resolution and then scale up. I was just now thinking that while(1)if (PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE)) {
if (msg.message != WM_DISPLAYCHANGE) { sleep(200);}break; would be better since technically the program wouldn't continue until the control panel was closed even if they applied the change.

drkwlf 23-Jan-15 4:12am

Also is it better, in your opinion, to let them change it themselves or for the Application to adjust the resolution... I like the idea of changing it through the program because it sounds difficult querying display modes and picking the one closest to the current resolution that is greater than the minimum. At the same time I wonder if it's ok to have a program change a system setting itself.

Richard MacCutchan 23-Jan-15 4:26am

Neither, your application should adjust its window to fit the resolution of the user's screen. You should never try to modify anything on a user's system, either manually or automatically. Any application that tries that on me goes straight into the bin.

drkwlf 23-Jan-15 4:35am

Your probably right, but by not doing either I wouldn't learn anything. So I will try it both ways. As far as the code itself, any opinions?

Richard MacCutchan 23-Jan-15 4:50am

I don't have the knowledge of those features to comment on whether it is right or wrong; only testing can show that.

I am not a fan of that style of putting the opening brace at the end of a conditional, I always put mine on the next line. However, that's just a personal preference, and there are some (strange) people who do it the way you do. Overall your layout looks pretty neat.

drkwlf 24-Jan-15 21:16pm

With the brackets... I taught myself programming and when I was figuring out statements they seemed to be opened with a function, member, loop etc... So my brain decided that logically thats where the bracket should be... I would assume your way would make it easier to find a missing bracket. Luckily that has not really been an issue for me. Like they say old habits die hard and I will try to remind myself to put the brackets on their own line... Thanks.

drkwlf 24-Jan-15 21:16pm

PS I am only a little strange... I think...

Richard MacCutchan 25-Jan-15 3:18am

Whether you put the braces on the same or the next line is purely a matter of personal preference. I always use the line below, in C, C++ and C#, but the same line in Java, as that is the accepted Java standard.

KarstenK 23-Jan-15 4:10am

I release such code, with two exceptions: some more braces in th if and no while loop. (Maybe some counting)

drkwlf 23-Jan-15 4:16am

It's probably that it's 3 in the morning but I don't understand your comment. Get rid of the while loop? Which one? If I get rid of the first one how do I check if when they changed the resolution it now meets the minimum. If I get rid of the second then whats the best way to pause the program and wait for the display to change?

2 solutions

Solution 1

There is nearly always a better way to design your layout.
How you do it will depend on the platform you are using.

If you are coding for the desktop, you could try things like tabs, sliding panels, containers that are hidden unless another control is triggered (e.g. check box), auto-scrolling, and so on.

If you are designing for the web, then try Googling "responsive design".

You should NEVER Ask a customer/user to change their resolution - firstly, because it will annoy them, secondly because in some environments (corporate, for instance), they might simply lack the rights to do so.

If, however, you are writing a game, which uses (for example) DirectX, and runs full screen, you can adjust the resolution yourself, so long as you make sure their device can handle it, have good error handling in place if it all goes horribly wrong, and you put all their toys (resolution) back where you found them when they exit the game. But that's the only exception I can think of. The rest of the time, it's really bad form to ask a user to change their environment just to run your app.

Posted 22-Jan-15 22:40pm

Out of Memory

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Jochen Arndt · Accepted Answer · 2015-01-22T22:49:00

Solution 2

As already noted by Richard your app should not insist on changing the resolution. You may check the resolution at program start and show a message like "Sorry. This application requires a screen resolution of x X y or higher" and terminate if it is too small.

The ugly part of your code is the loop calling FindWindowEx and Sleep without additional break condition.

When the screen resolution is changed by the user, Windows sends the WM_DISPLAYCHANGE[^] message to all windows. A better approach is handling this message. This will also detect a resolution change that occurs after your program has started.

Posted 22-Jan-15 22:49pm

Jochen Arndt

Updated 22-Jan-15 22:50pm

v2

Comments

drkwlf 24-Jan-15 21:09pm

More than likely I will remove that code 'after' I find the best way of doing it. This is just a hobby so I can tell you the end-user, "me", doesn't mind being asked if I want to change resolution :). Asking to change the resolution and opening the control panel is a complicated feat which makes it worth doing. First you need to know what OS your running on ( Windows Vista +) uses the IOPENCONTROLPANEL while Windows XP doesn't have an exact procedure for doing it... After which you have to find a way to pause the program until the resolution is changed and then recheck before continueing. Its problem solving which I enjoy and it's why I am doing this in the first place.

I agree with checking for WM_DISPLAYCHANGE... From experimentation with PeekMessage it seems as though Windows passes that directly through the Window Procedure and not through the message loop? ie.. I can trap it through the window procedure but trying to trap it through PeekMessage hasn't worked... Perhaps I'm doing it wrong... I'll let you know how that works out.

I've liked your response the best. It seems everyone has an opinion on what I should or should not do, but failed to address the question that was asked. Though you don't agree with asking to change the resolution you offered suggestions on fixing the code and I appreciate that.

Jochen Arndt 26-Jan-15 3:08am

Thank you for your feedback.

It was not clear from your question that this is not going to be released.

You probably tried to catch the message inside your main window? Than you won't get it using PeekMessage because that would only catch posted messages and not the ones which are send (not queued). The WM_DISPLAYCHANGE message is send to top level windows and posted to all others.

You should always use the Window Procedure to catch messages. Using PeekMessage should be avoided (misses non queued messages, higher system load).

What about a simple modal dialog box that can be closed with a Cancel button and closes itself upon the WM_DISPLAYCHANGE message with valid size?

drkwlf 26-Jan-15 21:48pm

Interesting idea. I was contemplating all the error messages from MessageBox()s to Dialog's in order to provide more information about that specific error. Using WM_DISPLAYCHANGE:{CheckResolution();EndDialog(0);} would be much neater code although that still leaves the problem of the cleanest way to hang the program until the resolution is changed.

Jochen Arndt 27-Jan-15 3:14am

The modal dialog box will block your program until it is closed either by a resolution change or by closing it manually.