Second Round CURL Wrapper Review

[jdrewsen]

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