1
\$\begingroup\$

I've spent some time on this as an answer elsewhere, and did my best to optimize it. But looking at the hill of indents seems like this can be improved. I've tried to implement an any() statement to replace the for break part, without any luck. Does anyone have any suggestions?

Groups = [[['NM1', 'OP', '1', 'SMITH', 'JOHN', 'PAUL', 'MR', 'JR'],
           ['ABC', '1L', '690553677'],
           ['DIR', '348', 'D8', '20200601'],
           ['DIR', '349', 'D8', '20200630']],
          [['NM1', 'OP', '1', 'IMA', 'MEAN', 'TURD', 'MR', 'SR'],
           ['ABC', '1L', '690545645'],
           ['ABC', '0F', '001938383',''],
           ['DIR', '348', 'D8', '20200601']]]

def ids(a, b):
    l = []
    for group in Groups:
        for lst in group:
            if lst[0] == a and lst[1] == b:
                if lst[2] == 'D8':
                    l.append(lst[3])
                else:
                    l.append(lst[2])
                break
        else:
            l.append(None)
    return l
        
current_id = ids('ABC', '1L')
prior_id = ids('ABC', '0F')
start_date = ids('DIR', '348')
end_date = ids('DIR', '349')
        
print(current_id)
print(prior_id)
print(start_date)
print(end_date)

Output:

['690553677', '690545645']
[None, '001938383']
['20200601', '20200601']
['20200630', None]

So basically, I have this Groups list, in that list are 2 nested lists.
Assuming the output lists are [x, y], the first nested list in Groups is x, and the second is y.

You see my function has two parameters, a and b.
x and y are determined when their corresponding nested list has a list that has a as the first index, and b as the second index.

If the nested list doesn't have a list that meets that requirement, the x or y will be None.


UPDATE: Is there any way to flatten the series of indents? At least a few?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Can you provide some context, like what this program does and other background information? \$\endgroup\$
    – Linny
    Jul 8 '20 at 2:00
  • \$\begingroup\$ @Linny I've added more context. \$\endgroup\$
    – user227321
    Jul 8 '20 at 2:31
  • 2
    \$\begingroup\$ please provide a) the full description of the data format; b) a description of what you were supposed to implement. we need both to check if your implementation is correct and to review whether there are better implementations. \$\endgroup\$
    – stefan
    Jul 8 '20 at 4:00
  • \$\begingroup\$ flatten - depends on your assignment. is the None output required? \$\endgroup\$
    – stefan
    Jul 8 '20 at 4:19
  • \$\begingroup\$ @stefan That's right. \$\endgroup\$
    – user227321
    Jul 8 '20 at 4:20
1
\$\begingroup\$

As we do not know the task given we cannot fully review. While the code itself looks reasonable there is some smell.

date

To me it is very smelly, that a function named id() returns a date

start_date = ids('DIR', '348')

This is also requiring a switch inside your function, so most probably there are two functions intermingled that should not be.

data structure

list() is a bad structure to search in. While this may be the data format you read or get passed, you most probably should convert it to a structure allowing direct lookup (converting once). The description of the current structure is missing, also we do not know if there are other functions accessing the data.

\$\endgroup\$
3
  • \$\begingroup\$ Why is ['690553677', '690545645'] a date? \$\endgroup\$
    – user227321
    Jul 8 '20 at 4:26
  • \$\begingroup\$ I did not claim that. Your code (see in my answer) clams that for a specifiq query. As we are still missing description of data and task I cannot say if the name is wrong or the implementation. But one is wrong. \$\endgroup\$
    – stefan
    Jul 8 '20 at 4:38
  • \$\begingroup\$ In your answer: "id() returns a date". \$\endgroup\$
    – user227321
    Jul 8 '20 at 13:58
0
\$\begingroup\$

Type Hints

Use these to direct users what types of values your functions accept, and what types of values are returned.

from typing import List

def ids(a: str, b: str) -> List[str]:
    ...

Meaningful names

I would rename l to results, as that better demonstrates what you're returning.

You code now looks something like this:

from typing import List

groups = [
    [
        ['NM1', 'OP', '1', 'SMITH', 'JOHN', 'PAUL', 'MR', 'JR'],
        ['ABC', '1L', '690553677'],
        ['DIR', '348', 'D8', '20200601'],
        ['DIR', '349', 'D8', '20200630']
    ],
    [
        ['NM1', 'OP', '1', 'IMA', 'MEAN', 'TURD', 'MR', 'SR'],
        ['ABC', '1L', '690545645'],
        ['ABC', '0F', '001938383',''],
        ['DIR', '348', 'D8', '20200601']
    ]
]

def ids(a: str, b: str) -> List[str]:
    results = []
    for group in groups:
        for lst in group:
            lst = list(filter(None, lst))
            if lst[:2] == [a, b]:
                results.append(lst[-1])
                break
        else:
            results.append(None)
    return results

if __name__ == '__main__':

    current_id = ids('ABC', '1L')
    prior_id = ids('ABC', '0F')
    start_date = ids('DIR', '348')
    end_date = ids('DIR', '349')

    print(current_id)
    print(prior_id)
    print(start_date)
    print(end_date)

Spaced out groups so I could understand the layout easier.

List slicing

Instead of checking the first two elements with different checks, slice the array and check that array against an array of the passed elements.

Simpler checking

Since you want to add the last element, instead of checking the length of the array then appending a specific index, just append the last element using [-1].

Filtering

I noticed there was a '' at the end of one of the lists. If you expect this type of data, you should filter it out since it isn't involved with the rest of the function.

lst = list(filter(None, lst))

This filters out every None type value in lst. Since an empty string is a None type, it gets removed.

Main Guard

This prevents the outside code from running if you decide to import this from another file / program.

\$\endgroup\$
2
  • \$\begingroup\$ Is importing List necessary? \$\endgroup\$
    – user227321
    Jul 8 '20 at 3:16
  • \$\begingroup\$ @user227321 For type hinting, yes. \$\endgroup\$
    – Linny
    Jul 8 '20 at 3:50

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.