4
\$\begingroup\$

I've created a simple little class called JSONSelector to be able to select relevant data from a JSON object (a dict in Python). Here's what I have so far - it's based off of a few SQL statements, NumPy methods, and JavaScript iterating methods:

class JSONSelector:
    def __init__(self, json):
        self.json = json

    def __str__(self):
        return "{}({})".format(type(self).__name__, str(self.json))
    __repr__ = __str__

    def select(self, selectors):
        if selectors == "*":
            return type(self)(self.json)
        else:
            temp = {}
            for sel in selectors:
                temp[sel] = self.json.get(sel, None)
            return type(self)(temp)

    def where(self, cond):
        temp = {}
        for key, value in self.json.items():
            if cond(key, value):
                temp[key] = value
        return type(self)(temp)

    def astype(self, type_):
        temp = {}
        for key, value in self.json.items():
            temp[key] = type_(value)
        return JSONSelector(temp)

    def sort(self, **kwargs):
        return type(self)(dict(sorted(self.json.items(), **kwargs)))

    def foreach(self, func):
        for key, value in self.json.items():
            if func(key, value) == False: break

    def print(self, func=None):
        if not func:
            print(self)
            return
        else:
            print(func(self))
            return

Here's a test case with an explanation:

a = JSONSelector({
    "stuff": 5,
    "seeded": 6,
    "number": "7",
    "more_stuff": 8
})

a.select("*") \  # Select all entries from the dict
    .where(lambda key,value: type(value) is int) \ # Where the value is an integer
    .sort(key=lambda x: x[1]) \ # Sort by the value
    .astype(str) \ # Convert the values to type str
    .foreach(lambda key, value: print(key + ": " + value)) # Loop over all entries

I'd like to know if any of my code is redundant, where I can shorten things up, and if anything is incorrect. Thank you in advance!

\$\endgroup\$
4
\$\begingroup\$

Minor, but I'd expand your foreach a bit to make it clearer that func is a side-effect function that happens to return an indicator. In its current form, it looks like the function is only being run for the purpose of the condition.

Something closer to:

def foreach(self, func):
    for key, value in self.json.items():
        should_continue = func(key, value)

        if should_continue == False:
            break

If you flipped the logic and had them return when they want to break instead though, you could make it read a little nicer:

def foreach(self, func):
    for key, value in self.json.items():
        should_break = func(key, value)

        if should_break:
            break

I'm not sure there's much benefit to using your print method. I believe it's convoluting the simple task of just passing the object to print. If the user wants to pass some function before printing, just let them do it.

As an example, what intuitively makes more sense to you?:

json.print(str)

or

print(str(json))

Personally, I find the latter to make more sense.

I'll also note, your returns in that function aren't necessary. You don't need an early return since the two paths of execution are exclusive from each other, and an implicit return happens at the end of the method anyway.

Finally, I don't think negating the condition in if not func helps readability. I've read negating conditions makes them generally more difficult to understand, and I agree with that. I avoid negating a condition like that unless I really want a certain order of the bodies for aesthetic purposes. I'd write it as:

def print(self, func = None):
    if func:
        print(func(self))

    else:
        print(self)
\$\endgroup\$
4
\$\begingroup\$

A few notes:

Firstly, I would add some docstrings to each function to document what the expected behaviour is.

In select when you're not doing any mutations (i.e. under the * case), it seems you could just return self. Is there any reason to make a new copy?

In select, where, astype instead of creating a temporary dict, you could use a dict comprehension instead, example:

def where(self, cond):
    return type(self)({key: value for key, value in self.json.items() if cond(key, value)})

In astype you're using JSONSelector, however everywhere else you're using type(self) this should be consistent whichever one you go for.

print seems like an unnecessary function, but if you keep it the return lines have no effect.

Hope that's helpful.

\$\endgroup\$
  • 1
    \$\begingroup\$ Taking a copy (even without doing mutations) is how Django does things as well - as that way you can mutate one of the results without mutating the copy. \$\endgroup\$ – Shadow Jun 20 at 3:30
  • \$\begingroup\$ @Shadow that makes sense and is consistent \$\endgroup\$ – bertilnilsson Jun 20 at 4:26

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.