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 MusicXML Haskell module can be found here: http://hackage.haskell.org/package/musicxml Thanks, Mike
Hi Michael It looks fine - with one big exception. Tracking down /undefined/ can be quite tedious, there is no indication at run time which particular undefined signalled the exit - I'd recommend you always throw an error with an identifiable string. Also, I wonder if the the MusicXML library on Hackage was auto-generated at least in part? It would be nicer to work with if it used constructors and field-labels more, rather than long tuples; though there isn't much you can do about that. Best wishes Stephen
Thanks, Stephen. I didn't realize I had left all those undefined's in there. I don't know of the origin of the MusicXML package. One nice thing is that reading the source for it, and looking at what fields go in the tuples, give a clue about the valid combinations of elements in MusicXML. Otherwise, I had at my disposal only some MusicXML tutorials, which don't go into depth. I supposed I could look at the XMD file or whatever that's called (the document that defines MusicXML) but I am not currently familiar with how to read those. Mike Stephen Tetley wrote:
Hi Michael
It looks fine - with one big exception.
Tracking down /undefined/ can be quite tedious, there is no indication at run time which particular undefined signalled the exit - I'd recommend you always throw an error with an identifiable string.
Also, I wonder if the the MusicXML library on Hackage was auto-generated at least in part?
It would be nicer to work with if it used constructors and field-labels more, rather than long tuples; though there isn't much you can do about that.
Best wishes
Stephen
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:
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
participants (3)
-
Heinrich Apfelmus -
Michael Mossey -
Stephen Tetley