HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Getting computer info

Submitted by: @import:stackexchange-codereview··
0
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 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.