Critique Win32 World Map App
Jimbo
I have made my 1st complete Win32 app & I am looking for criticism on
what should be done better.
Its nothing complex, it just shows a map of the world & a dot on the
city we are focused on, eg if we are focused on Sydney, a dot will
appear where sydney is on Australia on the world map, if you press
next, the focus will advance to the next city New York & a dot will
appear there & so on.
Can you tell me you would do better, any functions I could use that
would simplfy things etc. As u'll see I am pretty new to Win32.
The application will work if you compile it but u will need to create
2 bitmaps worldMapScaled.bmp & dot.bmp in order to compile.
Code:
/*
Application: Displays a world map & a dot at position of the city
we are focused(selected) on
Main Areas needing critique/advice:
- Bad variable names?
- Overuse of global variables?
ie, (sometimes global variables are passed as parameters in
functions, is that necessary?)
- Is there Efficient use of memory? What can be done better?
- Methods(& variables) used to store & access data? Suggestions
of better ways?
*/
#include <windows.h>
#include <stdio.h>
#include <vector>
#include <string>
#include <cstdlib>
using namespace std;
#define IDB_NEXT 1
#define IDB_PREV 2
#define IDT_CNAME 3
#define IDB_MAP 4
// Are sum of these UNECESSARILY global?
const char g_szClassName[] = "myWindowClass";
static HINSTANCE gInstance;
HWND cityBmp;
HBITMAP map = NULL; // handle to bitmap of world
HBITMAP cityImg = NULL;
BITMAP bm;
int xImgOffset, yImgOffset;
// Can you think of better ways to store & access data than these
below?
vector <string> cityList;
vector <int> xCoord;
vector <int> yCoord;
int index = 0;
string focus = "City Focus = ";
string city = "New York";
string str;
// Functions List //
LRESULT CALLBACK WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM
lParam);
// Is it smarter to make the following functions return type 'bool' so
I can detect errors
// with their execution? Or is void better?
void updateList(vector<string> &cityList);
void displayWin(HWND hwnd, HWND controls[]);
void setCityFocus(HWND hwnd, char type);
void drawCityBmp(HWND hwnd, HBITMAP img, int xOffset, int yOffset);
int WINAPI WinMain(HINSTANCE gInstance, HINSTANCE hPrevInstance, LPSTR
lpCmdLine, int nCmdShow)
{
WNDCLASSEX wc;
HWND hwnd;
MSG Msg;
//Step 1: Registering the Window Class
wc.cbSize = sizeof(WNDCLASSEX);
wc.style = 0;
wc.lpfnWndProc = WndProc;
wc.cbClsExtra = 0;
wc.cbWndExtra = 0;
wc.hInstance = gInstance;
wc.hIcon = LoadIcon(NULL, IDI_APPLICATION);
wc.hCursor = LoadCursor(NULL, IDC_ARROW);
wc.hbrBackground = (HBRUSH)(DKGRAY_BRUSH);
wc.lpszMenuName = NULL;
wc.lpszClassName = g_szClassName;
wc.hIconSm = LoadIcon(NULL, IDI_APPLICATION);
// if registration of main class fails
if(!RegisterClassEx(&wc))
{
MessageBox(NULL, "Window Registration Failed!", "Error!",
MB_ICONEXCLAMATION | MB_OK);
return 0;
}
// Step 2: Creating the Window
hwnd = CreateWindowEx(
WS_EX_CLIENTEDGE,
g_szClassName,
"Displays the Long & Lat of world cities",
WS_OVERLAPPEDWINDOW,
CW_USEDEFAULT, CW_USEDEFAULT, 600, 500,
NULL, NULL, gInstance, NULL);
if(hwnd == NULL)
{
MessageBox(NULL, "Window Creation Failed!", "Error!",
MB_ICONEXCLAMATION | MB_OK);
return 0;
}
updateList(cityList);
ShowWindow(hwnd, nCmdShow);
UpdateWindow(hwnd);
// Step 3: The Message Loop
while(GetMessage(&Msg, NULL, 0, 0) > 0)
{
TranslateMessage(&Msg);
DispatchMessage(&Msg);
}
return Msg.wParam;
}
// Step 4: the Window Procedure
LRESULT CALLBACK WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM
lParam)
{
HWND bitMap, btNext, btPrev, tfCityName;
// create array of handles to make passing as parameter easier
HWND controls[] = { bitMap, btNext, btPrev, tfCityName };
switch(msg)
{
case WM_CREATE:
{
// Load Bitmaps - World map & dot
map = (HBITMAP)LoadImage
(NULL,"worldMapScaled.bmp",IMAGE_BITMAP,0,0,
LR_CREATEDIBSECTION |
LR_DEFAULTSIZE | LR_LOADFROMFILE);
cityImg = (HBITMAP)LoadImage(NULL,"dot.bmp",IMAGE_BITMAP,
0,0,
LR_CREATEDIBSECTION |
LR_DEFAULTSIZE | LR_LOADFROMFILE);
// if load fails
if (cityImg==NULL || map==NULL) {
MessageBox(hwnd,"Failed to load
bitmaps","Error",MB_OK | MB_ICONERROR);
}
// get cityBmp offsets
GetObject(cityImg,sizeof(bm),&bm);
xImgOffset = bm.bmWidth/2;
yImgOffset = bm.bmHeight/2;
// Create GUI
displayWin(hwnd,controls);
}
break;
case WM_PAINT: // if I dont put this in bitmaps dont get
displayed for some reason
{
}
break;
case WM_COMMAND:
{
switch(LOWORD(wParam)) {
case IDB_NEXT:
{
// advance focus to next city
setCityFocus(hwnd,'n');
// call function to draw dot on new city
drawCityBmp
(hwnd,cityImg,xImgOffset,yImgOffset);
}
break;
case IDB_PREV:
{
// set focus to previous city
setCityFocus(hwnd,'p');
// call function to draw dot on new city
drawCityBmp
(hwnd,cityImg,xImgOffset,yImgOffset);
}
break;
default:
break;
}
}
break;
case WM_CLOSE:
DestroyWindow(hwnd);
break;
case WM_DESTROY:
PostQuitMessage(0);
break;
default:
return DefWindowProc(hwnd, msg, wParam, lParam);
}
return 0;
}
void updateList(vector<string> &cityList)
{
// Post: Stores list of cities in global vector
//
// When I increase the number of cities to display, I want to read
// the list of cites & their coords from a text file & build
vectors from the data
// from the text file.
// Is there a better/more efficient way?
cityList.push_back("New York");
// store coords of city
xCoord.push_back(185);
yCoord.push_back(140);
cityList.push_back("London");
xCoord.push_back(288);
yCoord.push_back(110);
cityList.push_back("Sydney");
xCoord.push_back(490);
yCoord.push_back(265);
}
void displayWin(HWND hwnd, HWND controls[])
{
// Post: Create & initialise interface controls
// Q: Why didn't I just write this straight in WM_CREATE?
// A: Well for readability, WM_CREATE looked messy with all the
code, so I though
// a function would clean it up - Do you think its necessary
HFONT hfDefault;
hfDefault = (HFONT)GetStockObject(DEFAULT_GUI_FONT); // set font
str = focus+city; // str="City Focus = New York"
controls[0] = CreateWindowEx(0,"Static","",WS_CHILD | WS_VISIBLE
| SS_BITMAP,
20,10,540,380,hwnd,(HMENU)
IDB_MAP,gInstance,NULL);
controls[1] = CreateWindowEx(0,"Static",str.c_str(),WS_BORDER |
WS_CHILD | WS_VISIBLE | SS_CENTER,
30,400,150,16,hwnd,(HMENU)
IDT_CNAME,gInstance,NULL);
controls[2] = CreateWindowEx(0,"Button","Prev",WS_BORDER |
WS_CHILD | WS_VISIBLE | BS_PUSHBUTTON,
410,400,70,20,hwnd,(HMENU)
IDB_PREV,gInstance,NULL);
controls[3] = CreateWindowEx(0,"Button","Next",WS_BORDER |
WS_CHILD | WS_VISIBLE | BS_PUSHBUTTON,
490,400,70,20,hwnd,(HMENU)
IDB_NEXT,gInstance,NULL);
// draw dot on focus city
cityBmp = CreateWindowEx(
0,"Static","",WS_CHILD | WS_VISIBLE | SS_BITMAP,
185-xImgOffset,140-yImgOffset,CW_USEDEFAULT,
CW_USEDEFAULT,hwnd,
NULL,gInstance,NULL);
// Set window fonts
SendMessage(controls[1],WM_SETFONT,(WPARAM)hfDefault,MAKELPARAM
(FALSE, 0));
SendMessage(controls[2],WM_SETFONT,(WPARAM)hfDefault,MAKELPARAM
(FALSE, 0));
SendMessage(controls[3],WM_SETFONT,(WPARAM)hfDefault,MAKELPARAM
(FALSE, 0));
// Display bitmaps
SendMessage(controls[0],STM_SETIMAGE,(WPARAM)IMAGE_BITMAP,(LPARAM)
map);
SendMessage(cityBmp,STM_SETIMAGE,(WPARAM)IMAGE_BITMAP,(LPARAM)
cityImg);
}
void setCityFocus(HWND hwnd, char type)
{
// Post: Increments or Decrements our focus city according to
type
// How would you write this function, I think it should be
written using better logic
// to determine whether we are incrementing or decrementing
// Type indicates whether we advance or decrement focus city
if (type=='n' || type=='N') {
// make sure index is not > size of cityList vector
if (index < cityList.size()-1) {
index += 1;
}
else index = 0;
}
else if (type=='p' || type=='P') {
if (index != 0) {
index -= 1;
}
else index = cityList.size()-1;
}
else {
MessageBox(hwnd,"Invalid parameter for SetCityFocus() funct
'type'","Error",
MB_OK | MB_ICONERROR);
return;
}
// append which city we are now focusing on
city = cityList.at(index);
str = focus+city;
// set window text - "City Focus is now ...."
HWND tfCityName = GetDlgItem(hwnd,IDT_CNAME);
SendMessage(tfCityName,WM_SETTEXT,(WPARAM)str.length()+2,(LPARAM)
str.c_str());
}
void drawCityBmp(HWND hwnd, HBITMAP img, int xOffset, int yOffset)
{
// Post: Draw dot bmp at location of current focus city
// destroy previous city bmp
DestroyWindow(cityBmp);
// create bmp coords by compensating for x,y offset
int xPos = xCoord.at(index)-xOffset;
int yPos = yCoord.at(index)-yOffset;
cityBmp = CreateWindowEx(
0,"Static","",WS_CHILD | WS_VISIBLE | SS_BITMAP,
xPos,yPos,CW_USEDEFAULT, CW_USEDEFAULT,hwnd,
NULL,gInstance,NULL);
HWND bmMap = GetDlgItem(hwnd,IDB_MAP);
// Redraw world map & city dot
SendMessage(bmMap,STM_SETIMAGE,(WPARAM)IMAGE_BITMAP,(LPARAM)map);
SendMessage(cityBmp,STM_SETIMAGE,(WPARAM)IMAGE_BITMAP,(LPARAM)
img);
}
Ulrich Eckhardt
> #include <windows.h>
> #include <stdio.h>
This is an old-style C header...
> #include <cstdlib>
...while this is a new-style header.
> using namespace std;
This is generally frowned upon, as it imports even "private" symbols
from 'std' into the global namespace.
> #define IDB_NEXT 1
> #define IDB_PREV 2
> #define IDT_CNAME 3
> #define IDB_MAP 4
Generally, use 'const' instead.
> const char g_szClassName[] = "myWindowClass";
I wouldn't bother storing this, just use it in the call to
RegisterWindowClass.
> static HINSTANCE gInstance;
Why is only this static?
Note: you have lots of globals, but since the whole program is written in a
procedural style, that is expected. Consider making them static or putting
them into an anonymous namespace to restrict their access a bit.
> vector <string> cityList;
> vector <int> xCoord;
> vector <int> yCoord;
struct city
{
std::string name;
int longitude;
int latitude;
};
std::vector<city> cities;
> int index = 0;
Can this be negative? If it's supposed to be a valid index in 'cityList',
note that its initial value isn't valid!
> string str;
A global variable with such a short name is probably a bad idea.
> // Is it smarter to make the following functions return type 'bool' so
> I can detect errors
> // with their execution? Or is void better?
Maybe rather throw an exception?
> void updateList(vector<string> &cityList);
Update the global list? In that case, no need to pass it around.
> void displayWin(HWND hwnd, HWND controls[]);
'controls' is a pointer to an array of unspecified length. This is
error-prone.
> wc.hbrBackground = (HBRUSH)(DKGRAY_BRUSH);
I'd hope that this cast isn't necessary.
> MessageBox(NULL, "Window Registration Failed!", ...);
Note: MessageBox() takes TCHAR strings. Take a look at MessageBoxA,
MessageBoxW and the MessageBox macro. This mistake doesn't cause errors
here and now, but it restricts portability.
> case WM_PAINT: // if I dont put this in bitmaps dont get
> displayed for some reason
> {
> }
> break;
Actually, all painting should take place here... I'm not entirely sure how
this works at all, I guess that controls paint themselves automatically
when the parent handled the WM_PAINT message.
> void updateList(vector<string> &cityList)
> {
> // Post: Stores list of cities in global vector
> //
> // When I increase the number of cities to display, I want to read
> // the list of cites & their coords from a text file & build
> vectors from the data
> // from the text file.
> // Is there a better/more efficient way?
>
> cityList.push_back("New York");
> // store coords of city
> xCoord.push_back(185);
> yCoord.push_back(140);
>
> cityList.push_back("London");
> xCoord.push_back(288);
> yCoord.push_back(110);
> cityList.push_back("Sydney");
> xCoord.push_back(490);
> yCoord.push_back(265);
> }
Hmm. If you use a fixed list of cities, consider doing this:
city const cities[] =
{
{"New York", 185, 140},
{"London", 288, 110},
};
size_t const num_cities = (sizeof cities)/(sizeof *cities);
Otherwise, I would create a single function "createCityList" that just fills
this vector (hardcoded or from a data file).
> void setCityFocus(HWND hwnd, char type)
Consider creating an enumeration for the type. Just reading the declaration
doesn't give me a clue what this parameter does.
I didn't compile or run your app, so I can't comment on whether it actually
works correctly.
Cheers!
Uli
--
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932