[Xmonad] more minor code comments on StackSet.hs

Michael Vanier mvanier at cs.caltech.edu
Tue Aug 7 19:57:30 EDT 2007


Here are some minor comments on the code in StackSet.hs, which I hope will be helpful to the 
authors.  Please excuse my extreme anal-retentiveness ;-)

In general, the code is very nice and very well-commented.  I have some gripes with the comments, 
though:

-- As is pointed out in the comments, what "master" means needs to be spelled out.  I use 
full/tabbed tiling only and I really don't understand the master concept very well, or how it 
relates to the data structures (especially since it's not explicitly represented in them).

-- Polymorphic data types (StackSet, Screen, Workspace) could do with comments specifying what the 
type parameters refer to.  Sometimes it's obvious, sometimes less so.

-- The comments are sometimes confusing wrt the complexity metrics. O(n), O(s), O(w) etc. are not 
always very intuitive in terms of what n, s, or w are supposed to be.  Seemingly "s" should be 
"screen" and "w" should be "workspace" or "window" but it isn't always that clear.  For
instance, "new" says it's O(n) and then it turns out that "n" is really "m".  "index" is O(s) even 
though it calls "integrate" which is O(n), and the focus/swap functions are O(w) (worst-case) where 
"w" means "window".

-- For "differentiate" the comment "texture a list" is not very illuminating.

Other comments:

-- I find the way that Screen wraps a workspace somewhat icky, though I can't think of a better 
alternative.  It feels like it compromises the symmetry of the StackSet data structure.  Maybe I'm 
just being too picky here.

-- Why do we need the StackOrNot type alias?  What's wrong with Maybe (Stack a)?  IMO the latter is 
more readable, or more readable when you use Maybe monad functions.

-- I think the "member" function would be simpler as

 > member :: Eq a => a -> StackSet i a s sd -> Bool
 > member a s = isJust $ findIndex a s

instead of

 > member a s = maybe False (const True) (findIndex a s)

-- I think the "insertUp" function would read better as

 > insertUp :: Eq a => a -> StackSet i a s sd -> StackSet i a s sd
 > insertUp a s = if member a s then s else insert s
 >   where insert = modify (Just $ Stack a [] []) (\(Stack t l r) -> Just $ Stack a l (t:r))

instead of

 > insertUp :: Eq a => a -> StackSet i a s sd -> StackSet i a s sd
 > insertUp a s = if member a s then s else insert
 >   where insert = modify (Just $ Stack a [] []) (\(Stack t l r) -> Just $ Stack a l (t:r)) s

since in the latter case "insert" looks like a function but isn't.

Again, these are tiny, tiny issues.  Overall the code is really nice.

Mike







More information about the Xmonad mailing list