9
\$\begingroup\$

I have a function that uses Windows Update Agent (WUA) API through COM using the win32com.client module, part of pywin32 package for retrieving the installed and the available (not yet installed) Windows Updates. For each update, name, url of the relative Microsoft documentation and category is displayed. Finally the function returns 2 dictionaries containing a name and category of the installed updates and the available updates (not yet installed).

This is the code I wrote, it compiles and produces the expected result:

import win32com.client
import win32con
import win32api
import pywintypes
import re

def enum_winupdates():
    wua = win32com.client.Dispatch("Microsoft.Update.Session")
    update_seeker = wua.CreateUpdateSearcher()
    # Search installed Software Windows Updates
    search_installed = update_seeker.Search("IsInstalled=1 and Type='Software'")
    updates_installed = win32com.client.Dispatch("Microsoft.Update.UpdateColl")
    print("\n[+] Enumerating installed Windows or Drivers' Updates...(if any)\n")
    installed_updates = []
    installed_categories = []
    available_updates = []
    available_categories = []
    installed_dict = {}
    available_dict = {}
    # compiles the regex pattern for finding Windows Update codes
    updates_pattern = re.compile(r'KB+\d+')
    for i in range(0, (len(search_installed.Updates))):
        # saves installed update name in a variable
        update_installed = search_installed.Updates.Item(i)
        for j in range(0, len(update_installed.Categories)):
            # extracts Windows Update code using regex
            update_code = updates_pattern.findall(str(update_installed))
            # saves installed update category in a variable
            category = update_installed.Categories.Item(j).Name
            print("[*] Name: " + str(update_installed) + " - " +
                  "url: " + "https://support.microsoft.com/en-us/kb/{}".format(
                "".join(update_code).strip("KB")) + " - " +
                  "Category: " + category)
            installed_updates.append(str(update_installed))
            installed_categories.append(category)
    # converts lists to tuples in order to be used as a dictionary key
    installed_hashable = tuple(installed_updates)
    installed_hashable_category = tuple(installed_categories)
    # creates category:update dictionary
    for update in installed_hashable:
        for category_update in installed_hashable_category:
            installed_dict[category_update] = str(update)

    # Searches available Software Windows updates not installed
    search_available = update_seeker.Search("IsInstalled=0 and Type='Software'")
    updates_available = win32com.client.Dispatch("Microsoft.Update.UpdateColl")
    print("\n[+] Enumerating available Windows or Drivers' Updates not installed...(if any)\n")
    for i in range(0, (len(search_available.Updates))):
        update_available = search_available.Updates.Item(i)
        for j in range(0, len(updates_available.Categories)):
            # extracts Windows Update code using regex
            update_code = updates_pattern.findall(str(update_available))
            # saves installed update category in a variable
            category = updates_available.Categories.Item(j).Name
            print("[*] Name: " + str(update_available) + " - " +
                  "url: " + "https://support.microsoft.com/en-us/kb/{}".format(
                "".join(update_code).strip("KB")) + " - " +
                  "Category: " + category)
            available_updates.append(str(update_available))
            available_categories.append(category)
    # converts lists to tuples in order to be used as a dictionary key
    available_hashable = tuple(available_updates)
    available_hashable_category = tuple(available_categories)
    # creates category:update dictionary
    for update in available_hashable:
        for category_update in available_hashable_category:
            available_dict[category_update] = str(update)
    return installed_dict, available_dict

I would kindly ask for a code review focusing on performance and DRY. Most of the code I used for finding the installed updates is the same used for finding the updates that aren't installed yet. I wonder if the code could be refactored in some way in order to avoid repetition and perhaps have a performance gain.

\$\endgroup\$
6
+50
\$\begingroup\$

Loop like a native

I suggest you have a look/read at Ned Batchelder's excellent presentation "Loop like a native".

In your case, any time you write:

for i in range(0, (len(search_installed.Updates))):
    # saves installed update name in a variable
    update_installed = search_installed.Updates.Item(i)

You should most probably write:

for update_installed in search_installed.Updates:
    # saves installed update name in a variable

If you are using objects that do not support this natively, it may be worth writting an adapter as suggested in Raymond Hettinger's "Beyond PEP 8" talk.

(In any case, please note that 0 as a first argument of range is useless, you can write range(foo) instead of range(0, foo))

Do less, less often

You perform this operation update_code = updates_pattern.findall(str(update_installed)) for each category even though the result will (as far as I can tell) not change from one category to another. It would probably make sense to do this out of the loop.

Also, you call str(update_installed) many times for the same update_installed. Again, it may be worth doing only once.

Do not repeat yourself

It seems that you are doing the same thing twice. Only a few parameters are different. You could define a function to handle this.

At this stage, the code looks like (and is highly untested, the variable names might be pretty bad and I may have introduced errors but you'll get the idea of what I was trying to achieve):

import win32com.client
import win32con
import win32api
import pywintypes
import re

def get_software_updates(update_seeker, installed):
    # Search installed/not installed Software Windows Updates
    search_string = "IsInstalled=%d and Type='Software'" % installed
    search_update = update_seeker.Search(search_string)
    _ = win32com.client.Dispatch("Microsoft.Update.UpdateColl")
    updates = []
    categories = []
    update_dict = {}
    # compiles the regex pattern for finding Windows Update codes
    updates_pattern = re.compile(r'KB+\d+')
    for update in search_update.Updates:
        update_str = str(update)
        # extracts Windows Update code using regex
        update_code = updates_pattern.findall(update_str)
        for category in update.Categories:
            category_name = category.Name
            print("[*] Name: " + update_str + " - " +
                  "url: " + "https://support.microsoft.com/en-us/kb/{}".format(
                "".join(update_code).strip("KB")) + " - " +
                  "Category: " + category_name)
            updates.append(update_str)
            categories.append(category_name)
    # converts lists to tuples in order to be used as a dictionary key
    hashable = tuple(updates)
    hashable_category = tuple(categories)
    # creates category:update dictionary
    for update in hashable:
        for category_update in hashable_category:
            update_dict[category_update] = str(update)
    return update_dict

def enum_winupdates():
    wua = win32com.client.Dispatch("Microsoft.Update.Session")
    update_seeker = wua.CreateUpdateSearcher()
    print("\n[+] Enumerating installed Windows or Drivers' Updates...(if any)\n")
    installed = get_software_updates(update_seeker, installed=True)
    print("\n[+] Enumerating available Windows or Drivers' Updates not installed...(if any)\n")
    available = get_software_updates(update_seeker, installed=False)
    return installed, available

Do less (again)

From the code above, it is clear that elements from updates are already string but we have still calling str on them. I expect this to be useless.

Also, I expect the hashable and hashable_categories tuples not to be useful.

You could simply write:

    # creates category:update dictionary
    for update in updates:
        for category_update in categories:
            update_dict[category_update] = update
    return update_dict

Proper string format

In :

            print("[*] Name: " + update_str + " - " +
                  "url: " + "https://support.microsoft.com/en-us/kb/{}".format(
                "".join(update_code).strip("KB")) + " - " +
                  "Category: " + category_name)

You use a mix of string concatenation of string formatting. This is highly confusing. Instead, you could simply write:

            print("[*] Name: {} - url: https://support.microsoft.com/en-us/kb/{} - Category: {}".format(
                update_str,
                "".join(update_code).strip("KB"),
                category_name))

Do things less often (again)

I missed it in the first place but you could do the join-and-strip part out of the loop:

        # extracts Windows Update code using regex
        update_codes = updates_pattern.findall(update_str)
        update_codes_stripped = "".join(update_code).strip("KB")

Data structure

I have troubles understanding what the returns data structure is supposed to be. A dictionnary mapping each category to a single update is a bit weird. Wouldn't it make more sense to map each category to a list of updates ?

I've tried to emulate some item/category hierarchy to see what was happening but I might have wrong assumptions about what your code is suppose to retrieve.

For the record, here is the corresponding fake function and the value returned:

def get_software_updates(update_seeker, installed):
    hardcoded_updates = {
        'update1': ['cat1a', 'cat1b', 'cat1c'],
        'update2': [],
        'update3': ['cat3a', 'cat3b', 'cat3c', 'cat4c']
    }
    updates = []
    categories = []
    update_dict = {}
    for update, hardcoded_categories in hardcoded_updates.iteritems():
        for category in hardcoded_categories:
            updates.append(update)
            categories.append(category)
    for update in updates:
        for category_update in categories:
            update_dict[category_update] = update
    print(update_dict)
    return update_dict

returning:

{'cat3a': 'update1', 'cat3b': 'update1', 'cat3c': 'update1', 'cat4c': 'update1', 'cat1b': 'update1', 'cat1c': 'update1', 'cat1a': 'update1'}

(What I find weird is the fact that all categories are somehow linked to the same update).

\$\endgroup\$
  • \$\begingroup\$ thanks for you suggestions. I understand I could avoid specifying 0 in the loop. I also see sometimes for loops of the type "for update in updates" instead of using "in range". Is it possible to use it in this context? I'm aware that I'm doing the same thing twice but I don't know how to fix it. Besides it could be useful to use object oriented programming but I'm not familiar with it yet. \$\endgroup\$ – Fabio Jul 26 '16 at 10:20
  • \$\begingroup\$ Nothing can prevent you from using range but it makes things longer, more complicated, mes complicated and less efficient than it could be... Please note that I've added a few details in my answer. \$\endgroup\$ – Josay Jul 26 '16 at 12:05

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.