Second Round CURL Wrapper Review

[Jonathan M Davis]

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 
correct length.

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 
lambda.

And as I mentioned before, all of these enforces which take a new 
CurlException, should be changed to use enforceEx after you've fixed 
CurlException's constructor.

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

p.curl.set(CurlOption.timevalue, timestamp.toUnixTime());

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 
necessary.

In Smtp's constructor, yo ushould probably create a variable for the toLowered 
url. e.g.

auto lowered = url.toLower();

That way, you avoid having to lower it twice if the else branch of the if-else 
is taken.

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 
Phobos.

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))
    copy.clear(option);

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 
EnumMembers!CurlOption).

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 
function pure.

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