patterncppMinor
Getting computer info
Viewed 0 times
computerinfogetting
Problem
I was trying to get my computer information and I came up with this code. It is working but I need opinions on how I am doing it. It is my first time to using
std::string to return strings. Is it good or is there another way around?struct USER
{
std::string ID;
std::string Username;
std::string PCName;
std::string OSVersion;
};
std::string GetOSVersion()
{
OSVERSIONINFOEX osvi;
ZeroMemory(&osvi, sizeof(OSVERSIONINFOEX));
osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
GetVersionEx((LPOSVERSIONINFOA) &osvi);
std::string Version = "Unknown Operating System";
std::string ServicePack = osvi.szCSDVersion;
if (osvi.dwMajorVersion == 5 && osvi.dwMinorVersion >= 1)
{
if (osvi.wSuiteMask & VER_SUITE_PERSONAL)
Version = "Windows XP Home Edition " + ServicePack;
else
Version = "Windows XP Professional " + ServicePack;
return Version;
}
return Version;
}
void InitUser(User* u)
{
char UsernameBuffer[UNLEN + 1];
DWORD UsernameBufferSize = sizeof(UsernameBuffer);
GetUserName(UsernameBuffer, &UsernameBufferSize);
u->Username = UsernameBuffer;
char ComputerNameBuffer[MAX_COMPUTERNAME_LENGTH + 1];
DWORD ComputerNameBufferSize = sizeof(ComputerNameBuffer);
GetComputerName(ComputerNameBuffer, &ComputerNameBufferSize);
u->PCName = ComputerNameBuffer;
u->OSVersion = GetOSVersion();
return;
}Solution
It seems to me that it would be better if the
So, your header would be something like this:
...and the implementation (.cpp, or whatever) file something like this:
Since you didn't seem to be using the
I'd also prefer to do initialization as initialization, so this:
...would be implemented more like:
...or (if you can use C++11):
I'd also probably add a stream insertion operator for the class (whatever you end up naming it), using code something like this (inside the class definition):
Finally, if you're going to do OS version information, you probably want to support more than the two OS versions you have right now. Given that there are lots of other ways to get this information, and all of them do a better job than this, nearly the only reasonable choices seem to me to be either expand coverage substantially to the point that it's at least somewhat useful, or else remove it completely.
User struct knew how to initialize itself, by putting the initialization code into a constructor.So, your header would be something like this:
struct User
{
User();
private:
GetOSVersion();
std::string Username;
std::string PCName;
std::string OSVersion;
};...and the implementation (.cpp, or whatever) file something like this:
std::string
User::GetOSVersion() {
// code from GetOSVersion here
}
User::User() {
// code similar to body of InitUser goes here
}Since you didn't seem to be using the
ID member of User, I removed it. Since the information isn't all about the user, but includes the OS version and such, it would probably be better to rename User to something more like SystemInformation.I'd also prefer to do initialization as initialization, so this:
OSVERSIONINFOEX osvi;
ZeroMemory(&osvi, sizeof(OSVERSIONINFOEX));...would be implemented more like:
OSVERSIONINFOEX osvi = { 0 };...or (if you can use C++11):
OSVESIONINFOEX osvi {};I'd also probably add a stream insertion operator for the class (whatever you end up naming it), using code something like this (inside the class definition):
friend std::ostream &operator<<(std::ostream &os, User const &u) {
return os << u.Username << " " << u.PCName << " " << u.OSVersion;
}Finally, if you're going to do OS version information, you probably want to support more than the two OS versions you have right now. Given that there are lots of other ways to get this information, and all of them do a better job than this, nearly the only reasonable choices seem to me to be either expand coverage substantially to the point that it's at least somewhat useful, or else remove it completely.
Code Snippets
struct User
{
User();
private:
GetOSVersion();
std::string Username;
std::string PCName;
std::string OSVersion;
};std::string
User::GetOSVersion() {
// code from GetOSVersion here
}
User::User() {
// code similar to body of InitUser goes here
}OSVERSIONINFOEX osvi;
ZeroMemory(&osvi, sizeof(OSVERSIONINFOEX));OSVERSIONINFOEX osvi = { 0 };OSVESIONINFOEX osvi {};Context
StackExchange Code Review Q#51367, answer score: 4
Revisions (0)
No revisions yet.