Review of std.net.isemail part 2

[Jacob Carlborg]

On 2011-04-02 23:33, Jonathan M Davis wrote:
> 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've added a static assert with a custom error message. I think  static 
asserts can give better error messages than template constraints.

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

Ok, I'll add a function, any idea for a good name?

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

I think it's easier to work with tabs than spaces so I planned to work 
with tabs and than the last thing I would do before submitting a pull 
request would be to convert the tabs spaces, just forgot that. I just 
did a simple search and replace in my editor with a regular expression 
to remove the 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.
>
> 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.

I've already removed these aliases (not tstring).

> - Jonathan M Davis


-- 
/Jacob Carlborg