55502f40dc8b7c769880b10874abc9d0

The get method just doesn't look very pythonic.

class Planet(object):
	all = []
	@classmethod
	def get(self, **kwargs):
		for p in self.all:
			match = True
			for key, value in kwargs.items():
				if p.__dict__[key] != value: 
					match = False
			print match
			if match:
				return p
		return None

Refactorings

No refactoring yet !

E40ca6b36dbee9e693a16f32f1941acd

micheal d.

September 8, 2008, September 08, 2008 18:05, permalink

No rating. Login to rate!

Quite frankly, i would like you to say more what it should do, but i presume 'all' is a list of dict-like objects.

class Planet(object):

	def __init__(self, all = [])
            self.all = all        

	@classmethod
	def get(self, **kwargs)
	    for elem in self.all:
                for key, value in kwargs.items():
		    match = elem.__dict__.get(key, not value) != value
                    if match: print match, p
                    else: print False
            return None
D41d8cd98f00b204e9800998ecf8427e

Patch

September 8, 2008, September 08, 2008 20:02, permalink

No rating. Login to rate!

List comprehensions are _sometimes_ faster (and more aesthetically pleasing), but it seems like it would be a lot less space efficient in this case. It also makes printing a bit ugly.

The only "big" things I see are:
- "<obj>.__dict__.get(key, not value) != value" does not work properly for value=None, unless I missed something. Thus you could just as well write "<obj>.__dict__.get(k)", I would think. Same bug, less code ... *shrug*
- Python functions return None by default (if nothing else is explicitly returned), so it isn't necessary to actually write that line. Unless you're into that "clarity" stuff.

class Planet(object):
    def __init__(self, all=[]):
        self.all = all

    @classmethod
    def get(self, **kwargs):
        for p in self.all:
            match = [False for k,v in kwargs.items()
                     if p.__dict__.get(k) != v]
            if False not in match:
                return p
64798a399a79ed8d34fa83ba0e61c1ac

ielectric.myopenid.com

September 28, 2008, September 28, 2008 18:58, permalink

No rating. Login to rate!

Untested:

class Planet(object):
    all = []

    @classmethod
    def get(cls, **kwargs):
        return not any([True for p in cls.all for k,v in kwargs.iteritems() if p.__dict__.get(k) != v])
814bd2e15b40a3e8b2fd98ea4233a5d2

pope

March 3, 2009, March 03, 2009 13:13, permalink

No rating. Login to rate!

@ielectric.myopenid.com: That is both amazing and ugly

4a3cb531d8b843df40093d82c0af79d0

satoru_Logic

March 12, 2009, March 12, 2009 04:30, permalink

No rating. Login to rate!

It seems that Planet.all is a list of namespaces.
And the get method return the first namespace that match what's given in kwargs.
Following is my version with a generator expression as argument to the built-in all function for the `match` test.

class Planet(object):
    all = []

    @classmethod
    def get(cls, **kwargs):
        for namespace in Planet.all:
            match = all(
                    namespace.__dict__.get(key, not value) == value
                    for key, value in kwargs.iteritems() 
                    )
            print match
            return namespace if match
        else:
            return None
D41d8cd98f00b204e9800998ecf8427e

bob

January 12, 2011, January 12, 2011 21:56, permalink

No rating. Login to rate!
class Planet(object):
    all = []

    @classmethod
    def get(cls, **kwargs):
        for namespace in Planet.all:
            match = all(
                    getattr(namespace, key, not value) == value
                    for key, value in kwargs.iteritems() 
                    )
            print match
            return namespace if match
        else:
            return None

Your refactoring





Format Copy from initial code

or Cancel