[Haskell-beginners] Re: general comments requested
Heinrich Apfelmus
apfelmus at quantentunnel.de
Sun Jul 18 08:31:34 EDT 2010
Michael Mossey wrote:
> I'm wondering if I could get really general comments on my program...
> like notes about language features I'm failing to take advantage of.
>
> This is part of a program to convert MusicXML to a simpler
> representation which I then manipulate. Most of the data is defined in
> MusDoc.Data:
>
> <http://hpaste.org/fastcgi/hpaste.fcgi/view?id=27679#a27679>
>
> and the functions are defined in MusDoc.FromXml:
>
> <http://hpaste.org/fastcgi/hpaste.fcgi/view?id=27678#a27678>
>
> A module called XmlAccess can access various interesting parts of the
> MusicXML document in a way that lets me ignore most of its complexity:
>
> <http://hpaste.org/fastcgi/hpaste.fcgi/view?id=27680#a27680>
I'm not familiar enough with the problem domain to say anything about
the large scale organization of your code.
But on a per-function scale, you have a few helper functions that are
best expressed with list comprehension and combinators like fold or
listToMaybe instead of general recursion. For instance, I recommend to
write
-- in module MusDoc.XmlAccess
headerScoreParts = ...
where
remainder = [part | Part_List_2 scorePart <- pl_s]
-- in module MusDoc.FromXml
xScorePart = ...
where ...
lookupMidiInstr scInstrId =
listToMaybe . filter ((scInstrId==) . A.midiInstrumentId)
In a sense, the Haskell Prelude and standard libraries come with a
superb "domain specific language for manipulating (some) lists" which is
much easier to understand and visualize than general recursion. :)
Concerning the MusicXML package itself, I think the use of 7-tuples in
the MusicXML package for data types like Score_Header is creepy. I do
admit that it has some interesting, yet ghastly appeal due to its
regularity, but frankly, I think this is bad design. For instance,
consider the following randomly chosen example
data Orientation_ = Orientation_1 | Orientation_2
deriving (Eq, Show)
read_Orientation_ :: Data.Char.String -> Result Orientation_
read_Orientation_ "over" = return Orientation_1
read_Orientation_ "under" = return Orientation_2
I am rather dissatisfied that the constructors are named Orientation_1
and Orientation_2 instead of Over and Under , and that
read_Orientation is not a member of a type class or thelike.
I can only hope that this code was generated automatically ...
Regards,
Heinrich Apfelmus
--
http://apfelmus.nfshost.com
More information about the Beginners
mailing list