Tjyylli

I currently use this code to extract the required GPU data. Can you review this method for efficiency or best coding practices?

Code

import wmi 

c = wmi.WMI() 

def gpu_check():

    gpu_ouy =["fu","7"]

    gpu_a =""

    gpu_b =""

    ass   = str(c.Win32_VideoController()[0])

    asss  = str(c.Win32_VideoController()[1])

    aq = ass.split()

    for x in range(len(c.Win32_VideoController())):

        ass   = str(c.Win32_VideoController()[x])

        saa = ass.split()

        saa.remove("instance")    

        saa.remove("of")    

        saa.remove("Win32_VideoController")  

        saa.remove("{")    

        saa.remove("AdapterCompatibility")      

        saa.remove("AdapterDACType") 

        while "=" in saa:

            saa.remove("=")  

        saa.remove("};")

        saa.remove("SystemName")

        saa.remove("Description")

        saa.remove("InstalledDisplayDrivers")

        saa.remove("DeviceID")

        saa.remove("DriverDate")

        saa.remove("Name")

        saa.remove("Status")

        saa.remove("Caption")

        saa.remove("InfFilename")

        saa.remove("ConfigManagerUserConfig")

        saa.remove("ConfigManagerErrorCode")

        saa.remove("InfSection")

        saa.remove("DriverVersion")

        if "CurrentScanMode" in saa:

            saa.remove("CurrentScanMode")  

        saa.remove("SystemCreationClassName")    

        saa.remove("CreationClassName")          

        if "CurrentRefreshRate" in saa:

            saa.remove("CurrentRefreshRate") 

        if "CurrentHorizontalResolution" in saa:

            saa.remove("CurrentHorizontalResolution")        

        if "CurrentVerticalResolution" in saa:

            saa.remove("CurrentVerticalResolution")   

        if "CurrentNumberOfColors" in saa:

            saa.remove("CurrentNumberOfColors")           

        if "MinRefreshRate" in saa:

            saa.remove("MinRefreshRate")             

        if 'colors";' in saa:

            saa.remove('colors";')                   

        if "CurrentBitsPerPixel" in saa:

            saa.remove("CurrentBitsPerPixel")      

        if "MaxRefreshRate" in saa:

            saa.remove("MaxRefreshRate")              

        saa.remove("VideoProcessor")        

        saa.remove("PNPDeviceID")           

        if "CurrentNumberOfColumns" in saa:

            saa.remove("CurrentNumberOfColumns")     

        if "CurrentNumberOfRows" in saa:

            saa.remove("CurrentNumberOfRows")         

        saa.remove("Monochrome")      

        saa.remove("Availability")       

        del saa[5]

        if '"Integrated' in saa:

            saa.remove('"Integrated')  

        if '"Internal";' in saa:

            saa.remove('"Internal";')        

        if 'VideoModeDescription' in saa:

            saa.remove('VideoModeDescription')             

        saa.remove('AdapterRAM')    

        if 'RAMDAC";' in saa:

            saa.remove('RAMDAC";')

            sqa =int(((int(str(saa[1]).replace(";",""))/1024)/1024)/1000)                  

            saa[1] = sqa        

        else:

            sqa =int(((int(str(saa[2]).replace(";",""))/1024)/1024)/1000)  

            saa[2] = sqa

        if 'Corporation";' in saa:

            saa.remove('Corporation";')            

        while "FALSE;" in saa:

            saa.remove("FALSE;")   

        gpu_a =""

        gpu_b =""

        if saa[6] =="0;": 

            gpu_a = str(saa[2]).replace('"',"")+" "+saa[3]+" "+saa[4]+" "+str(saa[5]).replace('";',"")+" "+str(saa[1])+" GB"

            gpu_ouy[0] = str(gpu_a)

        else:

            gpu_b = str(saa[2]).replace('"',"")+" "+saa[3]+" "+saa[4]+" "+str(saa[5]).replace('";',"")+" "+str(saa[6]).replace('";',"")+" "+str(saa[1])+" GB"

            gpu_ouy[1] = gpu_b

    return gpu_ouy

I fully agree with the answer by @BaileyParker. I have basically no idea what any of your variables are. Or what your code is actually supposed to achieve. And indeed, there is probably a better way to achieve what you want in this case. However, knowing how to do string and list parsing efficiently is also sometimes necessary, so this is how you could have done that better.

In your code, most of the lines are spent on removing unneeded strings from a list. Instead of manually removing each one of them (and even adding guards for the ones you know might not even exist in the list, hoping you don't miss any of those), just define a blacklist of terms you never want in your list and exclude them. If you choose a set as a data structure for that blacklist, you only need to iterate once over your list and checking if each element is in the blacklist is $mathcal{O}(1)$, making this algorithm $mathcal{O}(n)$.

Your code on the other hand is $mathcal{O}(n)$ for every single term you remove, because the whole list needs to be checked for the item in the worst case, and twice that if you first check if a term exists in the list with in, which is also $mathcal{O}(n)$. This makes your code $mathcal{O}(nk)$ with $n$ the list of methods and $k$ the number of terms you want removed.

blacklist = {"instance", "of", ...}



for methods in c.Win32_VideoController():

    methods = [m for m in str(methods).split() if m not in blacklist]

    # do something with it

Let's ignore efficiency. It took me five minutes to figure out what you were trying to do. In the kindest way possible, this is a mess. But let's fix that!

Your variable names are terrible. I have no idea what an ass is. What about an asss? Surely that's a bad idea, because it's very easy to mistake one for the other. Even if it wasn't, what are they? A variable name should describe what it holds. A bunch of random characters aren't helpful. Same goes for gpu_ouy. Is that a typo? What are gpu_a and gpu_b? What is aq? What is c? What is saa? None of these variables names help someone reading your code understand their function.

Don't use len in a for loop. The idiomatic python approach is to just iterate over the collection (and let it deal with lengths and indices). So instead of:

for x in range(len(c.Win32_VideoController())):

    ass = str(c.Win32_VideoController()[x])

Use this:

for controller in c.Win32_VideoController():

   # do something with controller...

   pass

Some random points before we get to the big issue:

• In sqa =int(((int(str(saa[1]).replace(";",""))/1024)/1024)/1000), use // instead of calling int so much. This does floor division. So int(int(x / 1024) / 1024) is the same as x // 1024 // 1024. But then you could just merge them: x // (1024 * 1024). But what are those numbers? I suspect you're converting from bytes to GB (expect 1000 MB = 1 GB isn't correct unless you're dealing with harddrives, which sometimes define the measurements like so). So instead use a constant: BYTES_PER_GB = 1024 * 1024 * 1000. Then do: int(vram_size_bytes) // BYTES_PER_GB.


• Fix your spacing and formatting. PEP8 your code!


• What is saa[1] above? You have some many magic indices that no one can discern without understanding the value of str(controller). Prefer giving things descriptive names (use variables, heck overuse variables to make things clear) to magic indices.


• 
  remove can fail (it raises a ValueError if the element isn't found in the list).


• You don't need to initialize gpu_a and gpu_b (ex. gpu_a = "" is unnecessary), because later you assign stuff to them. They will be initialized on first assignment.

But now the largest issue: you're doing a ton of extra work that you almost certainly don't have to do! I can't seem to find good docs for wmi's wrapping around this, but according to the microsoft docs, this probably returns an object of some type. That means instead of all of this convoluted and fragile string parsing you're attempting, you can probably get at the information you want by just doing controller.AdapterRAM // BYTES_PER_GB or controller.Description (where controller comes from for controller in wmi.WMI().Win32_VideoController()). Open up a Python REPL by running python3 and then run the following:

>>> import wmi

>>> controller = wmi.WMI().Win32_VideoController()[0]

>>> help(controller)

>>> dir(controller)

That should give you an idea of all of the information you can get from it. You don't need to be doing all that string parsing! The information is already available on properties of the object!

To add on to what @BaileyParker said, instead of phrasing the WMI string directly, you should use the wmi_property command to get the value of the property you are interested in. You can get a list of properties from the Microsoft documentation: Win32_VideoController

For example, you could do something like this:

import json

import wmi



controllers = wmi.WMI().Win32_VideoController()



gpu_data = list()



for controller in controllers:

   controller_info = {

        'Name': controller.wmi_property('Name').value,

        'HRes': controller.wmi_property('CurrentHorizontalResolution').value,

        'VRes': controller.wmi_property('CurrentVerticalResolution').value,

    }

   gpu_data.append(controller_info)



print json.dumps(gpu_data, indent=4)

On my machine it prints the output:

[

    {

        "VRes": 1080, 

        "Name": "NVIDIA GeForce GTX 1050", 

        "HRes": 1920

    }

]