3
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ Your struct USER name doesn't seem to match User* u, but I am assuming that's an oversight and they're really the same type. \$\endgroup\$ May 22, 2014 at 4:03

3 Answers 3

4
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$
  • Although there's no official naming standard for C++, yours isn't too common, and it's also used a bit inconsistently (particularly in InitUser(). Your variables and functions are in upper CamelCase, while they are commonly in either lower camelCase or snake_case.

    Although the naming is still up to you, it would be good to distinctively name variables and types. This of course doesn't apply to the WinAPI types, which have to stay as is.

  • void functions don't need a return at the end; they will always return automatically. It's only necessary if you need to return early.

  • Regarding InitUser(), why not just return a User or pass a reference? It's generally best to avoid passing raw pointers in C++ due to ownership concerns.

\$\endgroup\$
0
1
\$\begingroup\$

Looks perfectly fine to me, except you probably only need one return Version; at the end of the GetOSVersion() function. Also, I would consider a return; at the end of a void function unnecessary.

\$\endgroup\$
2
  • \$\begingroup\$ the last return version is if it is not windows xp. it will return the default value of Version which is "Unknown Operating System" \$\endgroup\$
    – user42916
    May 22, 2014 at 4:03
  • \$\begingroup\$ Yes, you need the last return, but not the one above that (inside the if block). \$\endgroup\$ May 22, 2014 at 4:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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