Review of std.net.isemail part 2

[Jonathan M Davis]

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.

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.

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?

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

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.

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.



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.

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.

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.

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.



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.

- Jonathan M Davis