George Sakkis wrote: > 1. As already noted, ISet is not really descriptive of what the class > does. How about RangeSet ? It's not that long and I find it pretty > descriptive. In this case, it would be a good idea to change Interval > to Range to make the association easier.
The reason I decided not to use the term Range was because that could be confused with the range function, which produces a discrete set of integers. Interval isn't a term used in the standard/built-in library, AFAIK, so I may stick with it. Is this sound reasoning? > 2. The module's "helper functions" -- which are usually called factory > functions/methods because they are essentially alternative constructors > of ISets -- would perhaps better be defined as classmethods of ISet; > that's a common way to define instance factories in python. Except for > 'eq' and 'ne', the rest define an ISet of a single Interval, so they > would rather be classmethods of Interval. Also the function names could > take some improvement; at the very least they should not be 2-letter > abbreviations. Finally I would omit 'eq', an "interval" of a single > value; single values can be given in ISet's constructor anyway. Here's > a different layout: First, as far as having the factory functions create Interval instances (Range instances in your examples), I was hoping to avoid Intervals being publically "exposed." I think it's cleaner if the end-programmer only has to deal with one object interface. I like the idea of lengthening the factory function names, but I'm not sure that the factory methods would be better as class methods. Consider a use-case: >>> import iset >>> interval = iset.ISet.lowerThan(4) as opposed to: >>> import iset >>> interval = iset.lowerThan(4) This is less typing and probably eliminates some run-time overhead. Can you list any advantages of making the factory functions class methods? > class Range(object): # Interval > > @classmethod > def lowerThan(cls, value, closed=False): > # lt, for closed==False > # le, for closed==True > return cls(None, False, value, closed) > > @classmethod > def greaterThan(cls, value, closed=False): > # gt, for closed==False > # ge, for closed==True > return cls(value, closed, None, False) > > @classmethod > def between(cls, min, max, closed=False): > # exInterval, for closed==False > # incInterval, for closed==True > return cls(min, closed, max, closed) I like the function names, but in my dialect, lessThan would be more proper. > class RangeSet(object): # ISet > > @classmethod > def allExcept(cls, value): # ne > return cls(Range.lowerThan(value), Range.greaterThan(value)) > > 3. Having more than one names for the same thing is good to be avoided > in general. So keep either "none" or "empty" (preferably "empty" to > avoid confusion with None) and remove ISet.__add__ since it is synonym > to ISet.__or__. I agree that "none" should be removed. However, some programmers may prefer to use +, some |, so I'd like to provide both. > 4. Intervals shouldn't be comparable; the way __cmp__ works is > arbitrary and not obvious. I agree that __cmp__ is being used arbitrarily. I wanted sorting to be easy, but there's other ways of doing that. I'll take your suggestion. > 5. Interval.adjacentTo() could take an optional argument to control > whether an endpoint is allowed to be in both ranges or not (e.g. > whether (1,3], [3, 7] are adjacent or not). Interval objects weren't meant to be public; adjacentTo is used by ISet for combinatory functions. I could look into ways to hide the class from the public, but hiding things never seemed Pythonic to me; someone may want to use them for some reason. Maybe pydoc can be made to not document certain objects. > 6. Possible ideas in your TODO list: > - Implement the whole interface of sets for ISet's so that a client > can use either or them transparently. That was my intention. I'll have to see which interface functions I'm missing. > - Add an ISet.remove() for removing elements, Intervals, ISets as > complementary to ISet.append(). > - More generally, think about mutable vs immutable Intervals and > ISets. The sets module in the standard library will give you a good > idea of the relevant design and implementation issues. Both good ideas. > After I look into the module's internals, I'll try to make some changes > and send it back to you for feedback. Thanks for your feedback, George. I look forward to any additional comments you come up with. -- http://mail.python.org/mailman/listinfo/python-list