3
\$\begingroup\$

Can I get a review of my code which compares 2 dictionaries and display unique changes.

import time
from random import randrange

temp_data = [{"Name":"SuperFish","KeyID":"SecretCodeFish"},
            {"Name":"CowMan","KeyID":"SecretCodeCow"}]

def check_data(Names):
    temp_key = "SpecialKey"
    temp_list = []
    for item in Names:
        Users = {
            'Name': item['Name'],
            'Status':(randrange(2))}
        temp_list.append(Users)
    return temp_list

def display_data(data):
    for i in data:
        print("===")
        print (i)
        print("===")

Prev = check_data(temp_data)
Curr = check_data(temp_data)
New = []

# Runs Repeteadly
while True:
    Curr = check_data(temp_data)
    time.sleep(3)
    for item in Curr:
        if item not in (Prev):
            New.append(item)
    if len(New)>0:
        Prev = Curr
        display_data(New)
        New = []
    else:
        print("NO CHANGES DETECTED")

I'm mainly wondering if there is a cleaner way to write my solution.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ It's doubtful that you should be using a dictionary the way that you are, and should be passing around a stronger type like a named tuple or class instance, but your question is too hypothetical to answer. Surely you're not actually doing what check_data is doing every three seconds, but you haven't described your real use case. \$\endgroup\$
    – Reinderien
    Aug 31 at 23:45
  • \$\begingroup\$ Welcome to Code Review! I have rolled back Rev 3 → 2. Please see What to do when someone answers. \$\endgroup\$ Sep 1 at 18:23
  • \$\begingroup\$ What's with all the temp data and hardcoded values? \$\endgroup\$
    – Mast
    Sep 3 at 12:34
5
\$\begingroup\$

General

Variable names and function arguments should be lowercase.

You should wrap your main procedure in a main function, as well as a if __name__ == '__main__' guard. More info.

Function and variable naming are important for readability of your code. The clearer and more accurate your naming, the quicker and easier your code can be understood. For example: check_data does not check any data.
Its argument names is not a list of names, but a list of dictionaries that have a name and an id.
If I only see that function signature, I would expect to be able to do for name in names and get one name (probably as str) after another. Instead I get a dict, that has a "Name" field. for item in names should be an indicator, that names should be called something like items.

Type hints also make your code easier to read and understand, use them. PEP 484 | Python Docs


check_data

It's not quite clear why this function does what it does. Documentation (comments / docstrings) as to why this function is implemented this way would help.

temp_key is never used, delete it.

In its current state, the function can easily be cut down to a single line using a list comprehension:

def change_data(items: list[dict]) -> list[dict]:
    return [{'Name': item['Name'], 'Status': randrange(2)} for item in items]

If you're not familiar with list comprehensions and generator expressions I would recommend learning about them. They're a useful (and often very efficient) tool.

But beware that a one-liner is not always the best option. If the creation of the dictionaries gets more complex it's probably better to move it to its own function.

Also note that 'Status': (randrange(2)) does not need as many parentheses: 'Status': randrange(2).


display_data

I'd recommend cutting this down to a single print statement per loop iteration:

def display_data(data: list) -> None:
    for i in data:
        print("===", i, "===", sep="\n")

or even better

def display_data(data: list) -> None:
    line_sep = "==="
    for i in data:
        print(line_sep, i, line_sep, sep="\n")

main

Your first initialization of curr is immediately overwritten inside the loop, delete it.

if item not in (prev), does not need parentheses: if item not in prev.

There are better (more readable and more efficient) ways to create new, which should be called new_items or changed_items:

  • List comprehension: new_items = [item for item in curr if item not in prev]
  • Functional approach: new_items = list(filter(lambda item: item not in prev, curr))

This also allows you to get rid of both new = [] assignments, as these approaches will create an empty list if there are no new items.

You can replace if len(new_items) > 0 by if new_items. More about truthy and falsy values in Python


Complete code

import time
from random import randrange

temp_data = [{"Name": "SuperFish", "KeyID": "SecretCodeFish"},
             {"Name": "CowMan", "KeyID": "SecretCodeCow"}]


def change_data(items: list[dict]) -> list[dict]:
    return [{'Name': item['Name'], 'Status': randrange(2)} for item in items]


def display_data(data: list) -> None:
    line_sep = "==="
    for i in data:
        print(line_sep, i, line_sep, sep="\n")


def main():
    prev = change_data(temp_data)

    while True:
        time.sleep(3)

        curr = change_data(temp_data)

        new_items = [item for item in curr if item not in prev]

        if new_items:
            prev = curr
            display_data(new_items)
        else:
            print("NO CHANGES DETECTED")


if __name__ == '__main__':
    main()

Approach

Please note that the current algorithm runs in O(n^2). Unfortunately dict is an unhashable type in Python. Therefore we cannot use sets to get around this, which would run in O(n). More on that approach here.

\$\endgroup\$

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.