patterncppMinor
Windows application that draws India's flag
Viewed 0 times
applicationflagindiathatwindowsdraws
Problem
I have written a small Windows application that draws India's flag in the window. I am very new to using Visual C++ and I want to understand if my code can be improved further.
```
#include "afxwin.h"
#include
class CMyApp : public CWinApp
{
public:
virtual BOOL InitInstance ();
};
class CMainWindow : public CFrameWnd
{
public:
CMainWindow();
protected:
afx_msg void OnPaint();
DECLARE_MESSAGE_MAP();
};
CMyApp myAPP;
BOOL CMyApp::InitInstance()
{
m_pMainWnd = new CMainWindow;
m_pMainWnd->ShowWindow(m_nCmdShow);
m_pMainWnd->UpdateWindow();
return TRUE;
}
BEGIN_MESSAGE_MAP (CMainWindow, CFrameWnd)
ON_WM_PAINT()
END_MESSAGE_MAP()
CMainWindow::CMainWindow ()
{
Create(NULL,_T("India's Flag"), WS_OVERLAPPEDWINDOW );
}
void IndiaFlag(CDC &dc, int x, int y)
{
dc.SetBkMode(TRANSPARENT);
CRect rect;
CPen pen(PS_SOLID, 1, RGB(0,0,0));
CPen *oldPen = dc.SelectObject(&pen);
{
CBrush brush(RGB(255,130,0));
CBrush *oldBrush = dc.SelectObject(&brush);
dc.Rectangle(x,y,x+200,(y+50));
}
{
CBrush brush(RGB(255,255,255));
CBrush *oldBrush = dc.SelectObject(&brush);
dc.Rectangle(x,(y+50),(x+200),(y+100));
CPen pen2(PS_SOLID, 1,RGB(0,0,255));
CPen *oldPen = dc.SelectObject(&pen2);
dc.Ellipse((x+75),(y+50),(x+125),(y+100));
double Nx,Ny;
for (int angle=0;angle<360; angle+=15)
{
int angle2 = angle;
double length = 25(cos(double(angle2 (3.14159265 / 180))));
double len2 = 25(sin(double(angle2 (3.14159265 / 180))));
int originX = (x+100);
int originY = (y+75);
Nx = originX + length;
Ny = originY - len2;
dc.MoveTo(originX,originY);
dc.LineTo(Nx,Ny);
}
}
{
CBrush brush(RGB(34,139,34));
CBrush *oldBrush = dc.SelectObject(&brush);
dc.Rectangle(x,(y+100),(x+200),(y+
```
#include "afxwin.h"
#include
class CMyApp : public CWinApp
{
public:
virtual BOOL InitInstance ();
};
class CMainWindow : public CFrameWnd
{
public:
CMainWindow();
protected:
afx_msg void OnPaint();
DECLARE_MESSAGE_MAP();
};
CMyApp myAPP;
BOOL CMyApp::InitInstance()
{
m_pMainWnd = new CMainWindow;
m_pMainWnd->ShowWindow(m_nCmdShow);
m_pMainWnd->UpdateWindow();
return TRUE;
}
BEGIN_MESSAGE_MAP (CMainWindow, CFrameWnd)
ON_WM_PAINT()
END_MESSAGE_MAP()
CMainWindow::CMainWindow ()
{
Create(NULL,_T("India's Flag"), WS_OVERLAPPEDWINDOW );
}
void IndiaFlag(CDC &dc, int x, int y)
{
dc.SetBkMode(TRANSPARENT);
CRect rect;
CPen pen(PS_SOLID, 1, RGB(0,0,0));
CPen *oldPen = dc.SelectObject(&pen);
{
CBrush brush(RGB(255,130,0));
CBrush *oldBrush = dc.SelectObject(&brush);
dc.Rectangle(x,y,x+200,(y+50));
}
{
CBrush brush(RGB(255,255,255));
CBrush *oldBrush = dc.SelectObject(&brush);
dc.Rectangle(x,(y+50),(x+200),(y+100));
CPen pen2(PS_SOLID, 1,RGB(0,0,255));
CPen *oldPen = dc.SelectObject(&pen2);
dc.Ellipse((x+75),(y+50),(x+125),(y+100));
double Nx,Ny;
for (int angle=0;angle<360; angle+=15)
{
int angle2 = angle;
double length = 25(cos(double(angle2 (3.14159265 / 180))));
double len2 = 25(sin(double(angle2 (3.14159265 / 180))));
int originX = (x+100);
int originY = (y+75);
Nx = originX + length;
Ny = originY - len2;
dc.MoveTo(originX,originY);
dc.LineTo(Nx,Ny);
}
}
{
CBrush brush(RGB(34,139,34));
CBrush *oldBrush = dc.SelectObject(&brush);
dc.Rectangle(x,(y+100),(x+200),(y+
Solution
Thanks to your method
It consists of 3 stripes: orange, white and green (top to bottom) and a sort of a wheel in the center. So, why don't you just say:
?
This code describes what your're drawing in fact. There are no brushes or pencils here, because if I ask you what the flag looks like, for sure you're not going to tell me about pencils and brushes and how GDI works. You'll probably say there are 3 stripes and a wheel in the center and that's what I want to hear.
So, why do you write the code like you're not going to tell anyone what happens in there?
Looking at your code, I'd say that all the names you give are terrible:
What does it mean? You're only going to have a single brush, so that's why you call it a "brush"? Nothing specific about it? If I see a name
An old brush? When I see
What is
And once again, something called
P.S. Procedures/Functions/Methods describe actions, so their names are traditionally
IndiaFlag, I guess you're drawing a flag of India. The problem is, I don't know what it looks like. OK, I gonna google it.It consists of 3 stripes: orange, white and green (top to bottom) and a sort of a wheel in the center. So, why don't you just say:
int const width = ...
int const height = ...
int const stripeHeight = height / 3;
drawStripe(0, 0, width, stripeHeight, Orange);
drawStripe(0, stripeHeight, width, 2 * stripeHeight, White);
drawStripe(0, 2 * stripeHeight, width, 3 * stripeHeight, Green);
drawWheel(width / 2, height / 2)?
This code describes what your're drawing in fact. There are no brushes or pencils here, because if I ask you what the flag looks like, for sure you're not going to tell me about pencils and brushes and how GDI works. You'll probably say there are 3 stripes and a wheel in the center and that's what I want to hear.
So, why do you write the code like you're not going to tell anyone what happens in there?
Looking at your code, I'd say that all the names you give are terrible:
CBrush brush(RGB(255,255,255));What does it mean? You're only going to have a single brush, so that's why you call it a "brush"? Nothing specific about it? If I see a name
brush 20 lines later, do you expect me to remember it is white or any specific at all?CBrush *oldBrush = dc.SelectObject(&brush);An old brush? When I see
oldBrush 20 lines later, do you expect me to remember what this brush is? Do YOU know why you need this oldBrush? I see no code where you switch back to it, so why do you even do it?dc.Rectangle(x,(y+50),(x+200),(y+100));What is
x here? Why y+50? What is 500? What is 200?CPen pen2(PS_SOLID, 1,RGB(0,0,255));And once again, something called
pen2. Is this pen any specific, or you're just telling me you're using 2 pens? Should I care? Etc.P.S. Procedures/Functions/Methods describe actions, so their names are traditionally
DoWhat. Your IndiaFlag violates the principle, because it says What.Code Snippets
int const width = ...
int const height = ...
int const stripeHeight = height / 3;
drawStripe(0, 0, width, stripeHeight, Orange);
drawStripe(0, stripeHeight, width, 2 * stripeHeight, White);
drawStripe(0, 2 * stripeHeight, width, 3 * stripeHeight, Green);
drawWheel(width / 2, height / 2)CBrush brush(RGB(255,255,255));CBrush *oldBrush = dc.SelectObject(&brush);dc.Rectangle(x,(y+50),(x+200),(y+100));CPen pen2(PS_SOLID, 1,RGB(0,0,255));Context
StackExchange Code Review Q#14397, answer score: 6
Revisions (0)
No revisions yet.