Review of std.net.isemail part 2

[Jonathan M Davis]

On 2011-04-02 09:26, Jacob Carlborg wrote:
> 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 say that pretty much every template should have a template constraint on 
it. I'm not sure that I've ever seen a template which was flexible enough that 
it would compile with anything you gave it, and if there's an argument that 
you can give it which won't compile with it, it should fail the template 
constraints, otherwise the error messags are much harder to understand and 
track down.

> > 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?

In most cases, no. However, I can easily believe that a fancier program would 
care. For instance, anything which wanted to list the various e-mail status 
codes and their associated strings would need to have the list of 
EmailStatusCodes and their associated strings.

I don't doubt that few programs will care. But it would be so simple to make 
it possible to get a string and from its corresponding status code without 
having an EmailStatus, that I see no reason to not make it possible to do so. 
It makes the module more flexible, and I expect that if the module is used 
enough, then a program at some point will want that capability.

> > 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?

We have only been using spaces in Phobos - no tabs - and I don't expect that 
to change, so code in Phobos shouldn't have tabs in it. Mixing tabs and spaces 
tends to ruin formatting and cause problems. And trailing spaces are just 
wasted characters.

Personally, I have vim set up to always highlight any tabs as well as trailing 
whitespace, so I notice it immediately - which is why I noticed in this case.

Typically, there's a way to get code editors to show extra whitespace and/or 
remove it when saving the file. They also typically have a way to convert tabs 
to spaces. How that works depends in your editor though depends on your 
editor.

If you're using vim, then :retab would turn the tabs into spaces and the 
command :s/\s\+$// will remove all trailing whitespace (and presumably that 
can be converted into an appropriate sed command, but I'm not well-versed in 
sed).

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

It doesn't matter as much, but I think that it still matters. Using 
unnecessary aliases makes it harder to read code - especially when they're 
aliases for well-known symbols. front, popFront, and back are standard Phobos 
functions which are part of one of the core concepts in Phobos, and they're 
what the rest of Phobos uses. You can't get much more standard than that. 
tstring at least makes the code shorter, and it's actually listed before it's 
used (unlike the other aliases), and I'm still torn on whether it should be 
there. But aliases which are simple renamings for standard Phobos functions 
are completely unnecessary and harm code maintainability for anyone else who 
reads the code.

- Jonathan M Davis