Review of std.net.isemail part 2

[Jacob Carlborg]

On 2011-04-01 11:01, Jonathan M Davis wrote:
> On 2011-03-29 14:06, Jacob Carlborg wrote:
>> I've made a few minor changes:
>>
>> * Renamed EmailStatusCode.Off ->  None and On ->  Any
>> * Added and clarified the documentation for EmailStatusCode.Any and None
>> * Updated the documentation
>>
>> Github: https://github.com/jacob-carlborg/phobos/tree/isemail
>> Docs: http://dl.dropbox.com/u/18386187/isemail.html
>
> Shouldn't isEmail have a template constraint which verifies that the e-mail
> variable is a valid string type? Also, it would probably be better to use Char
> instead of T for the type. It's clearer that way. I believe that that's what
> std.string does.

That's a good idea.

> I'd strongly consider making EmailStatus' public member variables private with
> getter properties (no setters obviously, since they're const), since you can't
> even assign to a const struct, and that could be annoying. For instance, you
> can't really put them in an array very well. By making them only have getter
> properties, you get the same effect but can still put the struct in arrays or
> reassign it if you need to. const member variables in classes isn't such a big
> deal, but I'm disinclined to use them in structs due to the issues that they
> cause.

Ok, didn't think about that.

> I'd be inclined to make EmailStatus' constructor private. I'm not sure that it
> makes any sense for it to be public. Why would anyone create one rather than
> using isEmail to create it?

I guess you're right.

> You should probably add a note to the documentation that the DNS check hasn't
> been implemented yet.

Yes.

> It would probably be a good idea to create a free function which takes an
> EmailStatusCode and returns the status string, move the logic in the status
> function to there, and have it call the new function. That way, if a program
> needed to know what the actual string was without having an e-mail with that
> particular error, it could get at the string.

Is this really necessary? Would you ever have the status code without 
the EmailStatus?

> If you're going to mention a version number, you might want to make it clearer
> what the code is based on, since from Phobos' perspective, the version number
> makes no sense. It _does_ make sense to say that it's based on version X of
> library Y but not that the module itself is version X - especially when that
> version is 3+, and it's brand-new.

Yeah, I kind of most of that text as is.

> Nitpicks:
>
> You have tabs and trailing space in the file. You shouldn't have either. And
> even if we wanted tabs, they were used inconsistently.

Forgot about converting the tabs to spaces? How can I easily find 
trailing spaces?

> Why did you create aliases for front, back, and popFront? That's a really odd
> thing to do and makes the code harder to understand. I would have considered
> that to fall in the "useless alias" category and best to be avoided.

I like those names better and though it wouldn't matter all the much 
since they're private.

> As stated before, all of your enums seem to start with a capital letter when
> the typical thing to do in Phobos at this point is to start with a lowercase
> letter.

Yes, didn't know what the naming convention were for enum members when I 
started.

> It would be nice if isEmail were broken up given its size, but it does seem
> like the sort of function which wouldn't break up very easily. The most
> obvious way to do that would be to create separate functions for each case
> statement, but given all of the variables involved, that wouldn't work very
> well. About the only sane way that I could think of doing that would be to
> create class or struct which held all of the logic and had all of those local
> variables as member variables. Then each of the cases could be broken up into
> separate functions. It would probably be declared internal to isEmail, and
> isEmail would construct one and then get the result out of it. That would
> allow you to break up the function. However, I'm not at all convinced that
> that would really be a better solution. It does seem a bit convoluted to do
> that just to break the function up. If it really helped the function's
> readability and maintainability then it would be worth it, but it might make
> it worse. So, I don't really like the size of the function, and that's a way
> that you could break it up, but unless you really think that that's a good
> idea, I'd just leave it as is. It's the sort of function that's generally
> stuck being long.

I agree with you, it's far too big but I don't know how I could break it 
up. It would be a lot easier if I've written the function myself but 
it's just a port of a PHP function.

> Overall, it looks pretty good. And as long as it works, it should be fine
> other than the few API issues that I mentioned, and they're not exactly
> enormous issues.

Thanks.

> - Jonathan M Davis


-- 
/Jacob Carlborg