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!

asked Jun 19, 2019 at 18:00
\$\endgroup\$

2 Answers 2

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)
answered Jun 19, 2019 at 20:55
\$\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.

dfhwze
14.1k3 gold badges40 silver badges101 bronze badges
answered Jun 19, 2019 at 19:55
\$\endgroup\$
2
  • 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\$ Commented Jun 20, 2019 at 3:30
  • \$\begingroup\$ @Shadow that makes sense and is consistent \$\endgroup\$ Commented Jun 20, 2019 at 4:26

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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.