3
\$\begingroup\$

The following excerpt is part of a method for recursively patching the properties of an ORM model and related models using a dictionary:

try:
    anhaenge = d['anhaenge']
except KeyError:
    self.logger.debug('anhaenge unchanged')
else:
    try:
        anhang = anhaenge['anhang']
    except KeyError:
        self.logger.debug('anhaenge.anhang unchanged')
    except TypeError:  # anhaenge is probably None
        for record in self.anhang:
            yield (record, PatchMode.DELETE)
    else:
        for record in self.anhang:
            yield (record, PatchMode.DELETE)

        if anhang:
            for anhang_ in anhang:
                with suppress(EmptyDict):
                    for record in Anhang.from_dict(
                            anhang_, immobilie=self):
                        yield (record, PatchMode.CREATE)

I currently violate the don't repeat yourself (DRY) principle, since I do the very same operation within the block handling the TypeError and at the beginning of the inner else block. How can I resolve this to have the code

for record in self.anhang:
    yield (record, PatchMode.DELETE)

only once?

Clarifying the input:

The inputd is a dictionary derived by json.loads() from a JSON string through a web API. The method, the above excerpt is extracted from uses this dictionary to modify database records represented by the ORM model this methods belongs to. Examples of d:

# Delete all subsequent anhang records
d = {'anhaenge': {
    'anhang': None}}

# Change all related anhang records to those represented by the dicts in
# the list, meaning:
# 1) Delete all old records
# 2) Create new ones from the dicts
d = {'anhaenge': {
    'anhang': [
        {'anhangtitel': 'Attachment1'},
        {'anhangtitel': 'Attachment2'}]}}

# All subsequent anhang records are left as they are.
# Subsequent foo records may be altered accordingly
d = {'anhaenge': {
    'foo': 'bar'}}
\$\endgroup\$
4
  • \$\begingroup\$ With the addition of clarifying the input, how should {'anhaenge': None} work? \$\endgroup\$
    – Peilonrayz
    Commented Nov 9, 2016 at 14:56
  • \$\begingroup\$ Remove this anhaenge record and all its related records (contains also other amongst anhang). This is essentially the reason for the TypeError block. \$\endgroup\$ Commented Nov 9, 2016 at 15:05
  • \$\begingroup\$ So the TypeError block does more than what you show here? \$\endgroup\$ Commented Nov 9, 2016 at 15:15
  • \$\begingroup\$ No. Within the outer else block, below the inner try... except... else... there come further try... except... else... blocks similar to this one for other sub-keys of anhaenge. \$\endgroup\$ Commented Nov 9, 2016 at 15:18

3 Answers 3

2
\$\begingroup\$

If we assume that either for loop can come before or after the other for loop. Then you should check if anhaenge is not None. As your except TypeError is to guard against that. You can then return from the function after you log the error, remove the except TypeError, remove the duplicate for loop, and finally, put the duplicate for loop after the if, as you wish to perform the loop here on either branch. EG:

try:
    anhaenge = d['anhaenge']
except KeyError:
    self.logger.debug('anhaenge unchanged')
else:
    if anhaenge is not None:
        try:
            anhang = anhaenge['anhang']:
        except KeyError:
            self.logger.debug('anhaenge.anhang unchanged')
            return
        else:
            for anhang_ in anhang:
                with suppress(EmptyDict):
                    for record in Anhang.from_dict(anhang_, immobilie=self):
                        yield record, PatchMode.CREATE
    for record in self.anhang:
        yield (record, PatchMode.DELETE)

But this is not possible if the duplicated for loop has to occur before the other for loop.

Either way, I honestly don't think you're input is right. Your input is strange, None is normally used to say it's not there, and so dict.get('a', None) is a common occurrence. However your code performs differently whether it's None or not there. And so I'd say you should restructure the input so you can use say:

anhang = d.get('anhaenge', {}).get('anhang', None)
if anhang:
    for record in self.anhang:
        yield (record, PatchMode.DELETE)

    for anhang_ in anhang:
        with suppress(EmptyDict):
            for record in Anhang.from_dict(
                    anhang_, immobilie=self):
                yield (record, PatchMode.CREATE)
else:
    self.logger.debug('anhaenge or anhaenge.anhang is unchanged')

If you can't adapt either of the above, then I'd say your code is good for your situation.

\$\endgroup\$
8
  • \$\begingroup\$ "But this is not possible if the duplicated for loop has to occur before the other for loop." - Which is exactly the case. Regarding the None - this indicates, that the subsequent content schould be deleted, while the absence of the key indicates that the subsequent content should not be changed at all. \$\endgroup\$ Commented Nov 9, 2016 at 13:58
  • \$\begingroup\$ @RichardNeumann And so I'd point you to my final paragraph. And repeat your input is odd. \$\endgroup\$
    – Peilonrayz
    Commented Nov 9, 2016 at 14:02
  • \$\begingroup\$ So how'd you suggest I'd do differ whether a field should be changed, deleted/nullified or not being touched? \$\endgroup\$ Commented Nov 9, 2016 at 14:10
  • 1
    \$\begingroup\$ @RichardNeumann I wouldn't post this question as it is on Stack Overflow. \$\endgroup\$
    – Peilonrayz
    Commented Nov 9, 2016 at 14:31
  • 1
    \$\begingroup\$ @RichardNeumann You could try Software Engineering, but be sure to check their help centre before asking. \$\endgroup\$
    – Peilonrayz
    Commented Nov 9, 2016 at 14:37
2
\$\begingroup\$

So, you want to delete old records if d['anhaenge']['anhang'] is present, whatever its value. And if it is not None, you want to update with new values as well.

The catch being that d may not contain the 'anhaenge' key, or the value for that key may be None. Looking at the except TypeError, the behaviour seems to be the same if you have d = {'anhaenge': None} and d = {'anhaenge': {'anhang': None}}. But d not containing 'anhaenge' should do nothing.

Since you say it's part of a method, you may extract a behaviour into its own function to benefit from early return: get a dictionnary of new values for 'anhang', or None if it shouldn't be changed:

def extract_anhang(d):
    try:
        anhaenge = d['anhaenge']
    except KeyError:
        self.logger.debug('anhaenge unchanged')
        return

    if anhaenge is None:
        return {}

    try:
        anhang = anhaenge['anhang']
    except KeyError:
        self.logger.debug('anhaenge.anhang unchanged')
        return
    return {} if anhang is None else anhang

and then, you can simplify your method into:

anhang = extract_anhang(d)
if anhang is not None:
    for record in self.anhang:
        yield (record, PatchMode.DELETE)

    for anhang_ in anhang:
        with suppress(EmptyDict):
            for record in Anhang.from_dict(anhang_, immobilie=self):
                yield (record, PatchMode.CREATE)

You can also use the same function to handle other subkeys similarly. Just parametrize it by the name of the subkey:

def extract_subkey(container, primarykey, subkey):
    try:
        subcontainer = container[primarykey]
    except KeyError:
        self.logger.debug('%s unchanged', primarykey)
        return

    if subcontainer is None:
        return {}

    try:
        subvalue = subcontainer[subkey]
    except KeyError:
        self.logger.debug('%s.%s unchanged', primarykey, subkey)
        return
    return {} if subvalue is None else subvalue

And the call become

anhang = extract_subkey(d, 'anhaenge', 'anhang')

In case you really prefer the TypeError approach (but I wouldn't use it as your comment suggest it is "probably" not the right approach), you can still use it instead of that if:

def extract_subkey(container, primarykey, subkey):
    try:
        subcontainer = container[primarykey]
    except KeyError:
        self.logger.debug('%s unchanged', primarykey)
        return

    try:
        subvalue = subcontainer[subkey]
    except TypeError:
        # subcontainer is probably None
        return {}
    except KeyError:
        self.logger.debug('%s.%s unchanged', primarykey, subkey)
        return
    return {} if subvalue is None else subvalue

In the end, nothing much changes, as this versions provide the same flow than yours, but early returns makes it more readable. You still have your two repeated blocks of code but they are now return {} and, even though they are linked to the logic, they don't contain some on their own; so it's better for maintainability.

\$\endgroup\$
0
\$\begingroup\$

If you still want to keep your logging you can use something like this.

anhaenge = d.get('anhaenge')

if anhaenge is None:
    self.logger.debug('anhaenge unchanged')
    return

anhang = anhaenge.get('anhang')
if anhang is None:
    self.logger.debug('anhaenge.anhang unchanged')
    return

for record in self.anhang:
    yield (record, PatchMode.DELETE)

for anhang_ in anhang:
    with suppress(EmptyDict):
        for record in Anhang.from_dict(
                anhang_, immobilie=self):
            yield (record, PatchMode.CREATE)
\$\endgroup\$
1
  • \$\begingroup\$ I currently differ between the absence of a key and it having a None value, beacuse it has a different meaning. See my comment on @Peilonrayz answer for details. \$\endgroup\$ Commented Nov 9, 2016 at 14:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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