7
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ What is c? Is c.Win32_VideoController() a simple accessor? \$\endgroup\$ – greybeard Apr 16 at 6:25
  • 2
    \$\begingroup\$ Welcome to Code Review. Do not comment comments asking for clarification: Edit your question, the code in it unless reviews have started. \$\endgroup\$ – greybeard Apr 16 at 6:32
  • 17
    \$\begingroup\$ ass is a very bad variable name. \$\endgroup\$ – 422_unprocessable_entity Apr 16 at 9:36
  • 21
    \$\begingroup\$ ass.split() is an even worse statement. \$\endgroup\$ – Raimund Krämer Apr 16 at 14:49
  • 3
    \$\begingroup\$ I like the Javascript spread operator though, ...ass \$\endgroup\$ – JollyJoker Apr 17 at 7:09
12
\$\begingroup\$

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
\$\endgroup\$
26
\$\begingroup\$

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.
  • As @IsmaelMiguel points out, not all cards support more than 1GB of RAM. This math will report 0 for them, which you may not want. One solution is to not use floored division (int(vram_size_bytes) / BTYES_PER_GB). When printing using the format specifier .02f so you don't get numbers with tons of decimal places (ex. "Video RAM: {:.02f}".format(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.
  • Given code this fragile, you definitely want to include tests. I'd imagine some integration tests would help. You can amass a list of outputs of str(c.Win32_VideoController()[0])s and manually do this string parsing. Then assert in some tests that your code outputs the thing that you expect for each. unittest will help you with this.

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!

\$\endgroup\$
  • 2
    \$\begingroup\$ Strictly speaking, one GB is 1000 MB and one MB is 1000 KB, but one GiB is 1024 MiB and one MiB is 1024 KiB. Going by the standards as defined by the IEC, that is. \$\endgroup\$ – vurp0 Apr 17 at 1:16
  • \$\begingroup\$ Won't this cause issues for GPUs with less than 1GB of vram? (For example, integrated GPUs can have anywhere between 8MB and 2GB. VirtualBox limits the maximum GPU memory to 128MB) \$\endgroup\$ – Ismael Miguel Apr 17 at 9:28
  • \$\begingroup\$ @vurp0 Sure, strictly speaking that's right. However, the "ibi" is often dropped (unfortunately) in common parlance. And as I mention, the only place where I've seen GB not mean GiB was in the context of hard drive storage (it's cheaper for manufacturers because 1 GB < 1 GiB, so they exploit this ambiguity). \$\endgroup\$ – Bailey Parker Apr 17 at 19:28
  • 1
    \$\begingroup\$ @IsmaelMiguel Yeah it will. This is a good point to call out. I'll append it to the discussion about the GB math. \$\endgroup\$ – Bailey Parker Apr 17 at 19:28
5
\$\begingroup\$

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
    }
]
\$\endgroup\$
  • 1
    \$\begingroup\$ I would make that for loop a list comprehension. Otherwise this seems to be the solution the OP actually needs in this case. \$\endgroup\$ – Graipher Apr 16 at 18:52
  • \$\begingroup\$ I'll add you to your code ram gpu,next gpu ram badly show controller.wmi_property('AdapterRAM').value \$\endgroup\$ – tadas Apr 17 at 6:09
  • \$\begingroup\$ You can (and should) spell list() as just [] \$\endgroup\$ – Bailey Parker Apr 17 at 19:52
  • \$\begingroup\$ @BaileyParker I disagree and intentionally make empty lists, dicts ect... that way to signal to the reader that they are intentionally created as empty structures differentiating them from structures created with content \$\endgroup\$ – Mark Omo Apr 18 at 2:16
  • \$\begingroup\$ What? How is an empty list on its own not signal enough that it is empty? [] is empty but if there is anything inside the brackets it’s not. \$\endgroup\$ – Bailey Parker Apr 18 at 2:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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