Windows application that draws India's flag

Question

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 <math.h>

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+150));
    }
}
void CMainWindow::OnPaint ()
{
    CPaintDC dc(this);
    IndiaFlag(dc, 150, 150);
}

---

In case you wondered, this is what the Indian flag looks like:

Answer 1

Thanks to your method 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.

Answer 2

Consider replacing any literal constants with symbols (macros, enums, functions). This should help understand the code better.

For example:

#define BLACK    RGB(0,0,0)

In combination with:

CPen pen(PS_SOLID, 1, BLACK)

Answer 3

IMO, this code seems ok!

To have a better clarity, you could create some functions for the drawing parts so that you India flag is made of smaller function. For example, for the inda flag, I would create a "drawColoredRectangle" and "drawCircleStar" just to make in clearer.

But for the rest, I think it's ok!