Second Round CURL Wrapper Review
Jonathan M Davis
jmdavisProg at gmx.com
Fri Dec 16 01:41:34 PST 2011
Please make sure that you remove trailing whitespace from the file. A lot of
the lines have trailing whitespace. Also, make sure that you don't have any
tabs in the file. There are a few places where you used tabs.
Line# 916 claims that the code there won't work and that it needs to be fixed,
so please fix it before it goes into Phobos (assuming that it's voted in).
On line# 1051, just do
auto arr = new Char(bufferSize);
There's no reason to create it and then assign to its length property.
The braces in the block follow line# 1131 need to be fixed. The indentation is
wrong on some of them, and not all of them are on their own line.
You should see if the cast(void)'s can be removed from byLineAsync. They
might still be necessary, but null was given a type with the last release, so
it might work without the cast now. The same goes with byChunkAsync.
Same with line# 1215 as 1051. There's no point in wasting a line of code by
declaring an array and the assigning to its length variable. The code is
probably slightly less efficient that way too. Just allocate a new array of the
On line#1297, the braces need to be fixed. Same with line# 1308 and line# 1334.
Just check over your braces in general to make sure that they're consistent
and are on their own line unless you're dealing with something like a one line
And as I mentioned before, all of these enforces which take a new
CurlException, should be changed to use enforceEx after you've fixed
The documentation on Protocol's isValid is wrong. It claims that isValid is
true if the instance is stoppend and invalid. Wouldn't that mean that the
object _wasn't_ valid?
There are quite a few places where you're concatenating several strings and
format would make the code much cleaner (e.g. Protocol's netInterface
function). Unless you're avoiding format in an attempt to allow functions to
be pure (since unfortunately, format can't currently be pure), you really
should be using format.
I'd argue that it's better to use empty than check whether a string is equal
to "". e.g. domain.empty instead of domain != "" on line# 1538.
There bodies of Http's constructor, static opCall, and dup functions are all
very similar - especially the constructor and static opCall. You should look
at refactoring those so that they don't have to duplicate so much code. Maybe
it can't be reasonably done easily, but it would certainly be better if that
code duplication could be reduced.
Change setTimeCondition to take a SysTime. DateTime is not intended for
timestamps. It's intended for calendar-based time, not the system time. Also,
SysTime makes it easier to convert to time_t. In general, the fact that you're
looking to deal with a time_t is a sign that you should be using SysTime. This
is especially true, since DateTime is going to give the wrong time_t in
general, since it has no time zone. As it stands, the timestamp is assumed to
be in UTC. That's just begging for bugs. Users won't expect that. Definitely
make it a SysTime. And once that's done, line# 2016 would disappear, and line#
2017 would become
On line# 2247, why do you use format with no format string? There should be no
~ or to!string in a call to format. It's just creating unnecessary memory
allocations. It should be something more like
format("%s%s(%s.", code, reason, majorVersion, minorVersion);
Though I find that '(' to be rather odd, since it has no matching closing
paren, so one may need to be added. Regardless, that line shouldn't be
concatenating strings or use conv.to. format takes care of all of that and
should do it with fewer memory allocations.
Pointers really should have their * next to the type not floating in space like
on line #2274. As it is, the code looks like a multiplication. Unlike in
C/C++, the * is clearly associated with the type. e.g.
int* a, b;
creates an int* and an int in C, but it creates int* and int* in D. Separating
the * from the type has a tendancy to make the code harder to understand.
Ftp's consturctor and static opCall have a code duplication problem similar to
that of Http.
Make encoding take a string. Any time that you are _always_ going to idup a
parameter which is a character array, make it a string, _not_ const. That way,
if it's a string, no idup is necessary, and if it's char, then it can be
iduped when it's passed in, and iduping only occurs when it's actually
In Smtp's constructor, yo ushould probably create a variable for the toLowered
auto lowered = url.toLower();
That way, you avoid having to lower it twice if the else branch of the if-else
Is p.curl.perform a property? If so, line# 2455 does nothing. If not, then it
needs parens. The curl module should be able to be built with -property. I
believe that Phobos as a whole is currently being built with tha flag. If not,
it will be soon, and this module will need to do that if it's merged into
If you can, please use a static foreach with EnumMembers!CurlOption in Curl's
dup. And if you can't, because you're not clearing all of them, then put all
of the ones that you _are_ clearing in an array (or TypeTuple if you want to
avoid the allocation for the array) and iterate over them with a foreach. e.g.
foreach(option; TypeTuple!(CurlOption.file, CurlOptions.writefunction))
You should be able to cut down on the number of lines by doing that
(_especially_ if you're actually clearing all of them and can use
Line#2639 is another case where format should be used. In general, if you're
doing more than one ~, consider using format unless you're trying to make a
I'd suggest renaming Message to something like CurlMessage. The odds of a name
clash with Message are likely high. And while they won't be able to use
Message, since it's private, it will still affect whether the full path for
Message must be given (e.g. my.module.Message instead of Message). CurlMessage
is far less likely to clash.
Rename DATA to Data. DATA does not follow Phobos' naming conventions. Granted,
it's private, but it's completely off. Type names should be pascal cased.
Why isn't empty a property on #3056? Sure, it's not a range, but it would be
more consistent with everything else if it were a property.
In general, you should use when converting between TickDuration and Duration,
but in some cases, no conversion should be necessary. For instance, line# 985
shouldn't need to do any converting. Duration's opOpAssign should be able to
handle a TickDuration.
Overall, the design looks fairly solid, and the code looks pretty good, but
there are definitely a number of minor issues which should be addressed before
this code makes it into Phobos. The biggest involve the functions' parameters
(such as using SysTime, not DateTime, and using string in some places where
you're using const(char) in order to avoid unnecessary idups). And those
definitely need to be sorted out sooner rather than later, since they affect the
API and will break code if they're changed later.
- Jonathan M Davis
More information about the Digitalmars-d