patterncppMinor
Simple wrapper class for Win32 console
Viewed 0 times
simplewin32wrapperforconsoleclass
Problem
This is based on a library called Consoledefender V 3.3.
I made a minimal class for personal usage based on this library by using C++11. It actually takes user input and prints it back on the console screen and acts like an echo. I would like this class reviewed.
Console.h
```
#include
#include
#include
#include
class Console
{
public:
Console();
~Console();
template
bool getinput(T& rNum, const short maxlen) const;
int wait(const std::string& rStrMessage = "Press any key to continue . . . ", const unsigned timeout = INFINITE) const;
bool setfontsize(short& rWidth, short& rHeight) const;
bool title(const std::string& rStrTitle) const;
bool resize(const short width, const short height) const;
bool move(const int left, const int top) const;
private:
static unsigned __stdcall key(void *ch);
DWORD getmode() const;
CONSOLE_SCREEN_BUFFER_INFO getscreenbufferinfo() const;
COORD getfontsize() const;
WINDOWPLACEMENT getplacement() const;
const HWND mHwndCon;
const HANDLE mOut, mIn;
const DWORD mMode;
const CONSOLE_SCREEN_BUFFER_INFO mConsoleSceenBufferInfo;
const COORD mFontsSize;
const WINDOWPLACEMENT mWindowPlacemet;
};
template
inline
bool Console::getinput(T& rStr, const short maxlen) const
{
return false;
}
template <>
inline
bool Console::getinput(int& rNum, const short maxlen) const
{
short len = 0, i = 0;
int numtmp = 0;
int ch = 0;
std::string str;
CONSOLE_SCREEN_BUFFER_INFO csbi = { 0 };
COORD coordBufSize = { maxlen, 1 };
COORD coordBufCoord = { 0, 0 };
if (maxlen chiVec(maxlen);
SMALL_RECT srctRect = { csbi.dwCursorPosition.X, csbi.dwCursorPosition.Y, static_cast(csbi.dwCursorPosition.X + maxlen), csbi.dwCursorPosition.Y };
COORD crdCur = csbi.dwCursorPosition;
if (!ReadConsoleOutputA(mOut, &chiVec.front(), coordBufSize, coordBufCoord, &srctRect))
{
return false;
}
for (auto& i : chiVec)
{
I made a minimal class for personal usage based on this library by using C++11. It actually takes user input and prints it back on the console screen and acts like an echo. I would like this class reviewed.
Console.h
```
#include
#include
#include
#include
class Console
{
public:
Console();
~Console();
template
bool getinput(T& rNum, const short maxlen) const;
int wait(const std::string& rStrMessage = "Press any key to continue . . . ", const unsigned timeout = INFINITE) const;
bool setfontsize(short& rWidth, short& rHeight) const;
bool title(const std::string& rStrTitle) const;
bool resize(const short width, const short height) const;
bool move(const int left, const int top) const;
private:
static unsigned __stdcall key(void *ch);
DWORD getmode() const;
CONSOLE_SCREEN_BUFFER_INFO getscreenbufferinfo() const;
COORD getfontsize() const;
WINDOWPLACEMENT getplacement() const;
const HWND mHwndCon;
const HANDLE mOut, mIn;
const DWORD mMode;
const CONSOLE_SCREEN_BUFFER_INFO mConsoleSceenBufferInfo;
const COORD mFontsSize;
const WINDOWPLACEMENT mWindowPlacemet;
};
template
inline
bool Console::getinput(T& rStr, const short maxlen) const
{
return false;
}
template <>
inline
bool Console::getinput(int& rNum, const short maxlen) const
{
short len = 0, i = 0;
int numtmp = 0;
int ch = 0;
std::string str;
CONSOLE_SCREEN_BUFFER_INFO csbi = { 0 };
COORD coordBufSize = { maxlen, 1 };
COORD coordBufCoord = { 0, 0 };
if (maxlen chiVec(maxlen);
SMALL_RECT srctRect = { csbi.dwCursorPosition.X, csbi.dwCursorPosition.Y, static_cast(csbi.dwCursorPosition.X + maxlen), csbi.dwCursorPosition.Y };
COORD crdCur = csbi.dwCursorPosition;
if (!ReadConsoleOutputA(mOut, &chiVec.front(), coordBufSize, coordBufCoord, &srctRect))
{
return false;
}
for (auto& i : chiVec)
{
Solution
A suggestion on architecture
I like the idea of your code and think it could be useful, however, it is very Windows specific, which limits its uses. The first thing I would think about, after putting the class together, would be separating the platform specific code from the portable code. The simplest and most straightforward way of doing this is by defining an interface class.
Code review
Now focusing on the presented code:
-
I don't like the idea of
-
Still talking about
-
Looking inside the
-
Again, inside the
-
-
You have a few hardcoded constants in some places, which are not obvious. Once again an excerpt from one of the
A comment would be adequate here.
-
All the
-
Instead of using the wacky `
I like the idea of your code and think it could be useful, however, it is very Windows specific, which limits its uses. The first thing I would think about, after putting the class together, would be separating the platform specific code from the portable code. The simplest and most straightforward way of doing this is by defining an interface class.
Console should be your interface. It might even have some methods implemented, it doesn't necessarily has to be a pure virtual interface, it could, for instance, make use of the template method pattern. Just make sure that you extract the platform specific code to a Win32Console or such class. No need to implement a UnixConsole now if you don't need one, just leave the code ready in case someone wants to expand on in the future (that might include yourself!).Code review
Now focusing on the presented code:
-
I don't like the idea of
getinput() as a template. You have provided four specializations of the function and left the generic one unimplemented and returning a false flag. You could have simply declared four overloads of the getinput() method for each type you wish to handle. In this case, calling the function for an unhandled type would produce a compiler error instead of an ambiguous runtime error.-
Still talking about
getinput(), it seems that the four overloads of this method share a lot of common code. The 10 first lines or so of each function seem identical. I would try to consolidate that repeated code into helper methods ASAP, if you haven't done this already. One possibility would be having a low-level helper that operates on a byte array and reads a number of bytes (e.g.: getInput(unsigned char * data, std::size_t numBytes). Then this low level function can be called with a buffer and the result reinterpreted into the proper type. This is much like a de-serialization routine, so it would be fine for a low-level helper to perform some unsafe type reinterpretation inside a safer type-aware method and in a controlled environment.-
Looking inside the
getinput() methods, you have several variables declared at the top of the functions. Remember to always declare variables as close to the first usage as possible. Also remember to apply const if it makes sense.-
Again, inside the
getinputs, instead of using &chiVec.front() to access a pointer to the underlaying data of a vector, use the data() method (C++11).-
std::istringstream is fine for number parsing, but it is a bit verbose and perhaps not so obvious. You can use the simpler std::stoi()/std::stof() for this task now.-
You have a few hardcoded constants in some places, which are not obvious. Once again an excerpt from one of the
getinputs:while (ch != 13)
{
ch = _getch();
if (ch == 0 || ch == 224)
{
ch = 256 + _getch();
}A comment would be adequate here.
13 could be replaced by its ASCII character literal, making it self-documented.-
All the
static_cast('+') (there are several of these) are not necessary at all. A char is automatically promoted to int, so the comparisons don't require the type cast.-
Instead of using the wacky `
API from Windows, switch to the Standard library. No porting effort there once you make the switch.
-
I'm not particularly fond of the tightlypacked naming convention. An _ between words or camelCasing would read easier to my eyes.
-
mIn / mOut are vague names. Call them properly by their functions: std-in and std-out.
-
Lastly, looking at your usage example:
Console console;
// Put window in the upper left corner of the screen
console.move(0, 0);
// Window size to a width of 100 and a height of 30 character Cells
console.resize(100, 30);
// Change Window Title
console.title("Test");
You are setting several parameters thru method calls soon after the creation of a Console. You should also offer the user the possibility of passing these configuration parameters as constructor arguments. And keep the empty constructor to create a default Console` for the "lazy" user.Code Snippets
while (ch != 13)
{
ch = _getch();
if (ch == 0 || ch == 224)
{
ch = 256 + _getch();
}Console console;
// Put window in the upper left corner of the screen
console.move(0, 0);
// Window size to a width of 100 and a height of 30 character Cells
console.resize(100, 30);
// Change Window Title
console.title("Test");Context
StackExchange Code Review Q#75509, answer score: 5
Revisions (0)
No revisions yet.