On Friday, 16 December 2011 at 09:42:50 UTC, Jonathan M Davis
wrote:
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.
ok
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).
ok
On line# 1051, just do
auto arr = new Char[](bufferSize);
There's no reason to create it and then assign to its length
property.
ok
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.
ok
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.
Still doesn't work.
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.
ok
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.
ok
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.
ok
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?
ok
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.
ok
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.
ok
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.
ok
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());
ok
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.
ok - ')' was missing
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.
If that's the D style I'll do that.
Ftp's consturctor and static opCall have a code duplication
problem similar to that of Http.
ok
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.
ok
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.
ok
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.
ok
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).
Nice.
I actually found out that you can do:
with (CurlOption)
foreach(option; TypeTuple!(file, writefunction))
copy.clear(option);
Which is pretty neat when you have to list as many options as in
the curl wrapper.
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.
ok
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.
ok
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.
ok
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.
ok
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.
ok
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
Thanks for the comments
/Jonas
