I said I would have another look at YAWS, given the apparently amazing performance. I decided to dive straight into the source, since I had more than a hunch that anything substantial written in Erlang would be an unmaintainable mass of crap. I certainly wasn’t disappointed in this case. The following excerpt is one of several chunks of code I came across that do exactly* the same thing:
%%% %%% Create W3CDTF (http://www.w3.org/TR/NOTE-datetime) formatted date %%% w3cdtf(GregSecs) -> "YYYY-MM-DDThh:mm:ssTZD" %%% w3cdtf(GregSecs) -> Date = calendar:gregorian_seconds_to_datetime(GregSecs), {{Y, Mo, D},{H, Mi, S}} = Date, [UDate|_] = calendar:local_time_to_universal_time_dst(Date), {DiffD,{DiffH,DiffMi,_}}=calendar:time_difference(UDate,Date), w3cdtf_diff(Y, Mo, D, H, Mi, S, DiffD, DiffH, DiffMi). %%% w3cdtf's helper function w3cdtf_diff(Y, Mo, D, H, Mi, S, _DiffD, DiffH, DiffMi) when DiffH < 12, DiffH /= 0 -> i2l(Y) ++ "-" ++ add_zero(Mo) ++ "-" ++ add_zero(D) ++ "T" ++ add_zero(H) ++ ":" ++ add_zero(Mi) ++ ":" ++ add_zero(S) ++ "+" ++ add_zero(DiffH) ++ ":" ++ add_zero(DiffMi); w3cdtf_diff(Y, Mo, D, H, Mi, S, DiffD, DiffH, DiffMi) when DiffH > 12, DiffD == 0 -> i2l(Y) ++ "-" ++ add_zero(Mo) ++ "-" ++ add_zero(D) ++ "T" ++ add_zero(H) ++ ":" ++ add_zero(Mi) ++ ":" ++ add_zero(S) ++ "+" ++ add_zero(DiffH) ++ ":" ++ add_zero(DiffMi); w3cdtf_diff(Y, Mo, D, H, Mi, S, DiffD, DiffH, DiffMi) when DiffH > 12, DiffD /= 0, DiffMi /= 0 -> i2l(Y) ++ "-" ++ add_zero(Mo) ++ "-" ++ add_zero(D) ++ "T" ++ add_zero(H) ++ ":" ++ add_zero(Mi) ++ ":" ++ add_zero(S) ++ "-" ++ add_zero(23-DiffH) ++ ":" ++ add_zero(60-DiffMi); w3cdtf_diff(Y, Mo, D, H, Mi, S, DiffD, DiffH, DiffMi) when DiffH > 12, DiffD /= 0, DiffMi == 0 -> i2l(Y) ++ "-" ++ add_zero(Mo) ++ "-" ++ add_zero(D) ++ "T" ++ add_zero(H) ++ ":" ++ add_zero(Mi) ++ ":" ++ add_zero(S) ++ "-" ++ add_zero(24-DiffH) ++ ":" ++ add_zero(DiffMi); w3cdtf_diff(Y, Mo, D, H, Mi, S, _DiffD, DiffH, _DiffMi) when DiffH == 0 -> i2l(Y) ++ "-" ++ add_zero(Mo) ++ "-" ++ add_zero(D) ++ "T" ++ add_zero(H) ++ ":" ++ add_zero(Mi) ++ ":" ++ add_zero(S) ++ "Z".
I’m sure I’m would be offending a number of people, were they to read this, but nonetheless I will say it – that’s absolutely shocking, and it’s fairly typical of the project in general. At some points in the source, there are slight redeeming features of some actual documentation headers above some of the functions, but they’re few and far between and incomplete when they do occur. All in all I’d say it either needs some dramatic clean-up and refactoring, or a good sprinkling with parmesan and black pepper to make a meal for a tramp’s dog.
I very much doubt that this is a YAWS issue, because the underlying OTP framework smells very similar, but I do live in hope of seeing a “well engineered” Erlang project. On the other hand, perhaps my assessment of the YAWS code is wrong. Disagreements are welcome.
Anyway, for now I am sticking with my opinion that it’s the “Erlang way of doing things” that’s interesting (and I mean the high-level design concepts obviously), and not Erlang itself.
*It’s somewhat difficult to verify the ‘exactly’ actually – suffice to say they ought to do the same thing. The above example came from yaws_rss.erl. See yaws.erl and yaws_log.erl for other examples.
-
Uh… I’m still pretty new to erlang, but I’m pretty sure what you’ve posted here is how one generally writes erlang (or any functional programming language) code.
The first function is the publicly exposed function and the rest are helper function variations. They do not do “exactly the same thing”.
Note that each of these helper functions has a “when” clause that matches certain conditions in the datetime being processed. Also note that they are separated by semi-colons, which means that the function definition continues.
Now, I think there’s a way to put all of those clauses under one function signature with one “when”… so you would just have a list of patterns to match with the logic to run for each.
But like I say, I’m still pretty new to erlang… not positive that’s possible. Either way, this code isn’t “wrong”… just more verbose.
-
I think you misunderstand my “do exactly the same thing”. This particluar piece of functionality, i.e. ISO date formatting, is replicated in serveral places in the project.
And I understood the code, thanks – despite appearances to the contraty I’m not a total imbecile.
-
I decided to edit the post and bold the word ‘one’ to prevent any futher misunderstandings.
-
It probably doesn’t help that this blog layout has hacked off the right-hand edge of the code, so you can’t see that, for example, the first two w3cdtf_diffs have identical contents, if you ignore a non-significant line break. So not only is identical code duplicated, but it’s been made to look different.
The guard statements (pardon my terminology if it’s wrong, I’m newer than you to Erlang I’m sure) are different on these two:
when DiffH < 12, DiffH /= 0 ->
vs
when DiffH > 12, DiffD == 0 ->Perhaps these is a reason why these can’t be easily combined, but I don’t know it yet. If they can’t, then the sensible thing to do would surely be to hand it off to another function. Either way, repeating the same complex expression twice AND making it look different by use of whitespace is exactly the kind of thing I’m moaning about.
-
Ok, sorry… missed the fact that this whole thing is replicated…. wow, that is pretty sucky.
6 comments
Comments feed for this article
Trackback link: http://ciarang.com/posts/yaws/trackback