[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