0
\$\begingroup\$

I have a method that takes a generator, converts it into a tuple to sort it, then into a list. It works perfectly, but being as I am fairly new to the Python world, I keep asking myself if it is worth doing or not...

@property
def tracks(self):
    if 'tracks' not in self.cache:
        s = Search(Track)

        q = {'query': self.name}
        if self.artists[0]['name'] != 'Various Artists':
            q.update({'artist': self.artists[0]['name']})
        _, tracks = s.search(**q)

        tracks = sorted(((int(i.track_number), i) for i in tracks))

        self.cache['tracks'] = [t for n, t in tracks]

    return self.cache['tracks']

I could care less about track_number as the track[i] already has that information, I'm just using it to sort the track list. s.search(**q) generates a generator of Track instances from a json object, I sort the track list, then I turn it back to a generator for an output.

Is there a better way? Should I just deal with the track number being there?

for _, track for Album.tracks:
    # work with track

Update:

The way the backend API works it only requires the first artist in the search, on a collaborated album of various artists, it just gives Various Artists, hence the self.artists[0]['name'] which is usually the only item in the list anyways.

And I know q['key'] = value is better practice than q.update({key: value}), originally it was taking more than a few items, hence the use of .update(), I just never changed that part.

The method is being called in Album.tracks where s = Search(Track) tells the Search class to use the Track class.

## models.py
class Album:

    @property
    def tracks(self):
        s = Search(Track)

## search.py
class Search(Service):

    def __init__(self, model):
        self.model = model

Instead of:

## models.py
class Album:

    @property
    def tracks(self):
        s = Search('track')


## search.py
from models import *

MODELS = {
    'album':  Album,
    'artist': Artist,
    'track':  Track
}

class Search(Service):

    def __init__(self, model):
        self.model = MODELS[model]

Search calls a backend api service that returns queries in JSON objects which then generates instances based on what self.model is set to. s = Search(Tracks) tells Search to call the track api and iterate through the results and return Track instances.

The main question here is, the current method that I have, is it doing too much? It generates the Track instances from the Search.search() generator, which is a somewhat abstract method for calling the api service for Album, Artist, and Track so it does nothing but generating instances based on what model it is given. Which is why I then have Album.tracks create a tuple so that I can sort the tracks base on track number, and then return a list of the tracks, nice and sorted.

Main point: Should I be worried about getting rid of the track numbers and just return the tuple, or is it fine to return the list?

Update 2:

class Album:

    @property
    def tracks(self):
        if 'tracks' not in self.cache:
            s = Search(Track)

            q = {'query': '', 'album': self.name}
            if self.artists[0]['name'] != 'Various Artists':
                q['artist'] = self.artists[0]['name']

            _, tracks = s.search(**q)

            self.cache['tracks'] = sorted(tracks, 
                key = lambda track: int(track.track_number))

        return self.cache['tracks']

class Track(BaseModel):

    def __repr__(self):
        artist = ', '.join([i['name'].encode('utf-8') for i in self.artists])
        track  = self.name.encode('utf-8')
        return '<Track - {artist}: {track}>'.format(artist=artist, track=track)

Calling it:

album_meta = Search(Album)

results = album_meta.search('making mirrors', artist='gotye')

for album results:
    print album.tracks

''' Output
[<Track - Gotye: Making Mirrors>,
 <Track - Gotye: Easy Way Out>,
 <Track - Gotye, Kimbra: Somebody That I Used To Know>,
 <Track - Gotye: Eyes Wide Open>,
 <Track - Gotye: Smoke And Mirrors>,
 <Track - Gotye: I Feel Better>,
 <Track - Gotye: In Your Light>,
 <Track - Gotye: State Of The Art>,
 <Track - Gotye: Don’t Worry, We’ll Be Watching You>,
 <Track - Gotye: Giving Me A Chance>,
 <Track - Gotye: Save Me>,
 <Track - Gotye: Bronte>] '''
\$\endgroup\$
  • \$\begingroup\$ This function would be easier to review if you told us a bit more about the Album class that it is defined on, what is it used for, what the performance considerations are, etc. \$\endgroup\$ – ruds Jun 15 '13 at 16:40
2
\$\begingroup\$

You create intermediate list for sorting:

tracks = sorted(((int(i.track_number), i) for i in tracks))

and then chose only one column:

self.cache['tracks'] =  [t for n, t in tracks]

I think this would be better to replace two statements above:

self.cache['tracks'] = sorted(tracks, key = lambda track: int(track.track_number))

Also, q.update({'artist': self.artists[0]['name']}) doesn't look good. It can easily be replaced with q['artist'] = self.artists[0]['name']

\$\endgroup\$
  • \$\begingroup\$ This is exactly what I was looking to hear. A way to reduce/combine it all. I felt like it was doing too much repetitive work generating a list to sort, then generate another list. \$\endgroup\$ – bnlucas Jun 15 '13 at 17:38
1
\$\begingroup\$

I'm not sure I understand what your question is. You already remove the track numbers from the value you return with:

self.cache['tracks'] = [t for n, t in tracks]

Other notes:

  • I'm a little worried about the line s = Search(Track). What is it searching? Does the constructor for Search have access to some sort of singleton database? This is a place where dependency injection would probably improve the code.
  • Why only self.artists[0]? In what circumstances would there be multiple artists? Why would you ignore results for the other artists? What if an artist participated in multiple albums with the same name but a different set of collaborators?
  • The q.update({...}) line is a bit odd. I would find it considerably clearer to read q['artist'] = ... instead.
\$\endgroup\$
  • \$\begingroup\$ The question has been updated with the in-depth explanation. \$\endgroup\$ – bnlucas Jun 15 '13 at 17:31

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.