please comment - my first classes

[Hilbert]

Thanks for your comments.

I'm using the lastKey, not the len() method because I want to be able
to delete entries. This way I can just delete the desired key.
I want to refer to bookmarks by their key, not by their value.
I'm not using lists for the same reason.

Thanks,
Hilbert

"Anton Muhin" <antonmuhin at sendmail.ru> wrote in message
news:b4t1q0$grt$1 at news.peterlink.ru...
> I quite agree with Sean about naming conventions too.
>
> Some more notes.
>
> > class bookmark:
> >  def __init__(self,group,name,url):
> >   self.group=group
> >   self.name=name
> >   self.url=url
> >
> >  def Print(self):
> >   print self.group,':',self.name,':',self.url
> it might be written as:
>       print ':'.join([self.group, self.name. self.url])
>
> if all are strings. This solution is quite understanable (at least for
> me ;) and might be a bit more effecient.
>
> >
> >  def AsTuple(self):
> >   return( (group,name,url) )
> >
> > class bookmark_group_db:
> >  def __init__(self):
> >   self.lastKey = -1
> >   self.groups = {}
> >
> >  def Add(self,name):
> >   self.lastKey += 1
> >   self.groups[self.lastKey]=name
> Or
>       self.groups[len(self.groups)] = name
>
> This seems to eliminate need for lastKey. BTW, why not to use lists?
>       self.groups = []
>       self.groups.append(name)
> ?
>
> >
> >  def Has(self,key):
> >   if self.groups.has_key(key):
> >    return 1
> >   else:
> >    return 0
> Maybe just:
>      return self.groups.has_key(key)
>
> or (for lists)
>      return 0 <= key < len(self.groups)
>
> >
> >  def PrintAll(self):
> >   for i in self.groups.keys():
> >    print i,':',self.groups[i]
> For dict:
>       for i, value in self.groups.iteritems():
>            print i, ':', value
>
> For lists (in Python2.3, but it quite easy for Python2.2 too):
>        for i, value in enumearte(self.groups):
>             print...
>
> >
> > class bookmark_db:
> >  def __init__(self):
> >   self.lastKey = -1
> >   self.bookmarks = {}
> >
> >  def Add(self,group,name,url):
> >   self.lastKey += 1
> >   self.bookmarks[self.lastKey]=bookmark(group,name,url)
> See above
>
> >
> >  def AddBookmark(self,src):
> >   self.lastKey += 1
> >   self.bookmarks[self.lastKey]=src
> See above
>
> >
> >  def PrintAll(self):
> >   for i in self.bookmarks.keys():
> >    self.bookmarks[i].Print()
> See above
> BTW, if you need this functionality often, why not abstract it into a
class?
>
> >
> >  def Find(self,sub):
> >   matches = bookmark_db()
> >   for i in self.bookmarks:
> >    if self.bookmarks[i].name.find(sub)>-1 or
> > self.bookmarks[i].url.find(sub)>-1:
> >     matches.AddBookmark(self.bookmarks[i])
> >   return (matches)
> I personally don't like () in returns, but it's a matter of taste
>
> Does this code work? Don't you mean:
>        for bookmark in self.bookmarks:
>              if bookmark.name.find(sub) ....?
>
> You may introduce function or method for bookmark matching:
>
>       def doesBookmarkMatch(bookmark, sub):
>             return bookmark.name.find(..) or bookmark.url.find(...)
>
> And use it.
>
> BTW,several more compact forms:
>       matches = [bookmark for bookmark in self.bookmarks if
> doesBookmarkMatch(sub)]
>
> or
>       matches = filter(doesBookmarMatch, self.bookmarks)
>
> >
> >  def Size(self):
> >   return (len(self.bookmarks))
> In some context, you might want to override __len__ method to use
> something like len(bookmark_db())
>
> BTW, your bookmark_db seems to be just a list. Maybe you need several
> functions and stick to a simple list.
>
> >
> [snipped]
>
> HTH,
> Anton.
>