4
\$\begingroup\$

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:

Indian flag

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ "I see no code where you switch back to it, so why do you even do it?" That's a bug. You must reselect the original object into the DC before releasing it. \$\endgroup\$ – Cody Gray Jan 25 '17 at 10:05
1
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ Good point! Didn't notice that at first read. \$\endgroup\$ – Jean-François Côté Aug 7 '12 at 15:00
  • \$\begingroup\$ Why don't just use CPen blackSolidPen(PS_SOLID, 1, BLACK)? It's not the case for #define. \$\endgroup\$ – Andrey Agibalov Aug 7 '12 at 17:38
  • \$\begingroup\$ This might be a bad example. Frankly, I prefer to stay away from #define, but it's the first thing I could think of. And I'm a lousy C/C++ programmer. I like to read it though. \$\endgroup\$ – Henk Langeveld Aug 7 '12 at 18:36
  • \$\begingroup\$ #define PI (3.14) is for C, because there is no const in C. In C++ there are real consts. \$\endgroup\$ – Andrey Agibalov Aug 7 '12 at 19:01
  • 2
    \$\begingroup\$ You don't need colors normally. You need "tools for drawing colorful things", like pencils and brushes (blackSolidBrush). Even in case you think you really want it, the "more C++" approach is like this: COLORREF const blackColor = RGB(0, 0, 0);. COLORREF is in fact just a number. \$\endgroup\$ – Andrey Agibalov Aug 7 '12 at 19:14
-3
\$\begingroup\$

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!

\$\endgroup\$
  • 2
    \$\begingroup\$ For sure this code doesn't seem to be any OK. \$\endgroup\$ – Andrey Agibalov Aug 7 '12 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.