3
\$\begingroup\$

As a C++ beginner learning C++ and DirectX at the same time, I was wondering if a more experienced C++ developer would be able to go over the code and tell me what is done incorrectly or something that you would change.

Window.h

#pragma once

#include <string>
#include <windows.h>

namespace Graphics {

class Window
{
private:
    const char *m_Title;
    int m_Width;
    int m_Height;
    HINSTANCE m_hInstance;
public:
    Window(const char *title, int width, int height);
    ~Window();

    bool Init();
    bool CreateD3DWindow();
    void Loop();

    inline int GetWidth() { return m_Width; }
    inline int GetHeight() { return m_Height; }
};
}

Window.cpp

#include "Window.h"

#include <iostream>

namespace Graphics {

// Forward declarations
LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);

Window::Window(const char *title, int width, int height)
    : m_Title(title), m_Width(width), m_Height(height)
{
}

Window::~Window()
{

}

bool Window::Init()
{
    if (!CreateD3DWindow())
    {
        return false;
    }

    return true;
}

void Window::Loop()
{
    MSG msg;
    ZeroMemory(&msg, sizeof(MSG));

    while (msg.message != WM_QUIT)
    {
        if (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
        {
            TranslateMessage(&msg);
            DispatchMessage(&msg);
        }
        else
        {
            // Render
        }
    }
}

bool Window::CreateD3DWindow()
{
    HWND hWnd;
    WNDCLASSEX wc;

    m_hInstance = GetModuleHandle(NULL);

    // Window styling and options
    wc.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
    wc.lpfnWndProc = WndProc;
    wc.cbClsExtra = 0;
    wc.cbWndExtra = 0;
    wc.hInstance = m_hInstance;
    wc.hIcon = LoadIcon(NULL, IDI_WINLOGO);
    wc.hIconSm = wc.hIcon;
    wc.hCursor = LoadCursor(NULL, IDC_ARROW);
    wc.hbrBackground = (HBRUSH)GetStockObject(BLACK_BRUSH);
    wc.lpszMenuName = NULL;
    wc.lpszClassName = m_Title;
    wc.cbSize = sizeof(WNDCLASSEX);

    if (!RegisterClassEx(&wc))
    {
        std::cout << "RegisterClassEx failed!" << std::endl;
        return false;
    }

    hWnd = CreateWindowEx(
        WS_EX_APPWINDOW,
        m_Title,
        m_Title,
        WS_OVERLAPPEDWINDOW,
        GetSystemMetrics(SM_CXSCREEN) / 2 - m_Width / 2,
        GetSystemMetrics(SM_CYSCREEN) / 2 - m_Height / 2,
        m_Width,
        m_Height,
        NULL,
        NULL,
        m_hInstance,
        NULL
    );

    if (!hWnd)
    {
        std::cout << "Failed to create window" << std::endl;
        PostQuitMessage(0);

        return false;
    }

    ShowWindow(hWnd, SW_SHOW);

    return true;
}

LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
{
    switch (message)
    {
    case WM_CLOSE:
        PostQuitMessage(0);
        return 0;

    case WM_KEYDOWN:
    {
        switch (wParam)
        {
        case VK_ESCAPE:
            PostQuitMessage(0);
            return 0;
        }
    }

    default:
        return DefWindowProc(hWnd, message, wParam, lParam);
    };

    return 0;
}
}
\$\endgroup\$
  • \$\begingroup\$ This is a little hard to review out of context because we have no idea on how the class is meant to be used. For example, my gut feeling looking at this is that Init(), CreateD3DWindow() and Loop() should all be private and invoked from the construtor (so that it basically behaves like std::thread), but I don't know enough about your situation to determine if that's right here or not. \$\endgroup\$ – Frank Nov 22 '17 at 4:35
  • \$\begingroup\$ @Frank The purpose of this Window class is to create a Window for an Engine that I am developing. What this means is that it's going to handle rendering etc. \$\endgroup\$ – ZOulhadj Nov 22 '17 at 8:02
  • \$\begingroup\$ Actually this class is rather useless. This class does not even manage lifetime of the created window or registered window class. All class methods are basically the same as usual init functions from default desktop application template, e.g. InitInstance -> Window::Init(). \$\endgroup\$ – VTT Nov 22 '17 at 10:00
2
\$\begingroup\$

I have one big swooping recommendation that I feel you should address before tackling anything else:

Mind your class invariants!

When designing a class, ideally you want to ensure every single public member function handles correctly any possible valid state of the class, in conjunction with any possible passed argument. In your case, ask yourself:

What happens if Loop() gets called before Init()? What happens if Init() gets called twice in a row? What happens if Create3DWindow() is called after Init()?

A good way to tackle that is to define what's known as a class invariant.

First, jot down on paper a description of the legal internal states of the class. That's known as the class invariant.

For example, you may want to have m_hInstance is either NULL or points to a window instance in your invariant definition, but m_hInstance points to a window instance would be even better, because it allows you to rely on that being true, so you don't have to constantly check.

Second, for each public member function, make sure that the function's behavior is well defined for every possible invariant state, and that it leaves the class in an invariant state after it's done.

Mind you, it doesn't mean that every public member function must "work", just that it doesn't explode, cause memory leaks, etc...

Edit: There are some extreme exceptions to this, such as std::vector<>'s operator[]. But this is an exception, and the class' documentation makes it extremely clear that you enter no-mans land if you call that with an index outside of the bounds.

\$\endgroup\$
  • \$\begingroup\$ Thank you for going over the code and telling what I need to improve! Much appreciated. \$\endgroup\$ – ZOulhadj Nov 22 '17 at 18:04
1
\$\begingroup\$

This feels like OOP for the sake of OOP. You don't actually manage the lifetime of the window, you don't hold on to a handle of your window, you don't forward the callback to your class' object,...

You can use SetWindowLongPtr(hWnd, GWLP_USERDATA, this); to store a pointer into the handle which can be retrieved using GetWindowLongPtr(hWnd, GWLP_USERDATA); And then cast to Window* so you can forward the call.

You aren't actually doing any D3D stuff yet. That part will be using COM. This is somewhat non-standard C++. Do some research on that. There are several ways to interact with that; make sure you pick a sane option and stay consistent with it.

\$\endgroup\$

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.