[Haskell-beginners] Re: general comments requested

[Heinrich Apfelmus]

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