2

I need to verify if 'info' is a json file or a python dict. Given the fact that the json file and the python dict has the same structure I wrote this code to parse both and save their content in a variable but I think there is a pythonic and more efficient code.

import json

class LoadInfo(object):
def __init__(self, info=None):
    if info:
        try:
            self.config= json.loads(info)
        except ValueError:
            print('Load Python Dict')
            try:
                if isinstance(info, dict):
                    self.config= info
            except ValueError:
                print('Json or python dict config is needed')
    else:
        raise Exception('Json or python dict config is needed')

info  = LoadInfo('path/to/json_file') #should work
info_dict = dict('A'=1, 'B'=2, 'C'=3)
info2 = LoadInfo(info_dict) #Also should work

Anyone has more ideias?

2
  • 1
    IMO you should restructure your code. Why can't you simply load the data outside LoadInfo class and pass the result to it? Why LoadInfo should accept two different types under one param? Commented Jul 10, 2017 at 21:23
  • 4
    Why would you write code that either takes a dictionary or a JSON string? I would write a classmethod def from_json(cls, info): return cls(json.loads(info)). That way you only deal with the dictionary case in __init__. Also you should never just raise Exception:; if the info parameter is required, don't give it a default value. Commented Jul 10, 2017 at 21:24

2 Answers 2

5

First things first, don't just raise Exception; that's far too general, be as specific as you can about what has gone wrong. In this case, it's that the user hasn't provided the info parameter. Two problems with that:

  1. You should test for None by identity, not truthiness (otherwise e.g. {} would be an exception, which may not be what you actually want): if info is not None:.

  2. If it's a required parameter, why give it a default value?!

Revision 1:

import json

class LoadInfo(object):

    def __init__(self, info):
        try:
            self.config = json.loads(info)
        except ValueError:
            print('Load Python Dict')
            try:
                if isinstance(info, dict):
                    self.config = info
            except ValueError:
                print('python dict config is needed')

(Note minor style guide tweaks.)


Next, there's really no need to provide this kind of polymorphism by allowing either a dictionary or a JSON string as the argument. As in the second case you're just parsing it to the first case, make that a class method, which is the common alternate constructor pattern in Python.

Revision 2:

import json

class LoadInfo(object):

    def __init__(self, info):
        try:
            if isinstance(info, dict):
                self.config = info
        except ValueError:
            print('python dict config is needed')

    @classmethod
    def from_json(cls, info):
        return cls(json.loads(info))

Which part of:

if isinstance(info, dict):
    self.config = info

do you expect to raise a ValueError? And why, in the case where it's not an acceptable type of input, would you want to just print something and let the program continue? Note that where you're checking types, it's better to use the ABCs.

Revision 3:

from collections.abc import Mapping
import json

class LoadInfo(object):

    def __init__(self, info):
        if not isinstance(info, Mapping):
            raise TypeError('mapping config is needed')
        self.config = info

    @classmethod
    def from_json(cls, info):
        return cls(json.loads(info))

But actually, you're suggesting that it be loaded from a file, not the JSON string as your current code implies (you provide 'path/to/json_file' not '{"foo": "bar"}' - it's not clear what you expected json.loads to make of that). So you need to handle that file.

Revision 4:

from collections.abc import Mapping
import json

class LoadInfo(object):

    def __init__(self, info):
        if not isinstance(info, Mapping):
            raise TypeError('mapping config is needed')
        self.config = info

    @classmethod
    def from_json_file(cls, filename):
        with open(filename) as json_file:
            return cls(json.load(json_file))  # note 'load' not 'loads'

Now your examples become:

info = LoadInfo.from_json_file('path/to/json_file')
info_dict = dict(A=1, B=2, C=3)  # note you shouldn't use quotes for keys here
info2 = LoadInfo(info_dict)
Sign up to request clarification or add additional context in comments.

Comments

-1

You need to open a file object at first if you pass a file and it will be better to separate file and string arguments:

import os
import json

class LoadInfo(object):
    def __init__(self, info=None, file=None):
        if file and os.path.exists(file):
            with open(file) as f:
                data = f.read()
            try:
                self.config = json.loads(data)
            except ValueError:
                raise ValueError('Load JSON file error')
        elif info:
            if isinstance(info, dict):
                self.config = info
            elif isinstance(info, str):
                try:
                    self.config = json.loads(info)
                except ValueError:
                    raise ValueError('Load JSON string error')
        else:
            raise ValueError('Load config error')

I would split it into two methods:

class LoadInfo(object):
    def load_from_file(self, file):
        with open(file) as f:
            data = f.read()
        self.config = json.loads(data)

    def load_from_str(self, info):
        if isinstance(info, dict):
           self.config = info
        elif isinstance(info, str):
            self.config = json.loads(info)
        else:
            raise ValueError('Load config error')

But actually, it's more pythonic to use ducktyping style.

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.