Re: Ep 19 "mistakes"

[Michael Azerhad]

Law of Demeter warns about train wreck involving different objects, meaning several distinct dependencies.

In this case:  name.trim().split("\\s+"),   although looking like a train wreck, it's still apply to the former object: name that is a String.

String being immutable, this could lead to the illusion of a violation of Demeter, but IMHO, it's not since it doesn't involve other dependencies than String.

Michael

On Tuesday, June 11, 2013 4:14:55 PM UTC+2, Norbert Nemes wrote:

It seems that my last post didn't made it to the forum.
I'm kind of new to the Google forum UI, so sorry if my current post causes
duplication.

Many "mistakes" have already been found by others, but how about the
"train wreck" that is splitNames(). Weren't you advising against this form in
one of your earlier episodes?
I also didn't like that all the methods in the NameInverter class have the same
visibility.
Not to mention that you "forgot" the Ms., Sir, HRM, Rev. (and possibly Sir Mr. 
- as in Sir Mr. Stewart - for obnoxious convention Treckies ;)) honorifics.

And while we are talking mistakes...
Data's communicator should be on the left side of his chest not on the right.

Anyway, it was a very nice episode :).

[Uncle Bob]

Winner #15.

Norbert,

The trainwreck in splitNames looks like this:

return new ArrayList<String>(Arrays.asList(name.trim().split("\\s+")));

The problem with trainwrecks (long chains of dots and parentheses) is that they know too much.  They are harmful when the things that they know are likely to change.  But nothing in this particular trainwreck is likely to change, nor is there a better way create the resultant ArrayList.  Merely breaking the trainwreck apart by introducing variables doesn't solve the problem of too much knowledge.  

I agree with the visibility issue.  Some of those methods should have been private.  The 'invertName' function should have been public.  

But the most egregious mistake is the misplacement of Data's communicator.  I'll deal with that one immediately.

[Norbert Nemes]

Well, on closer inspection I noticed that the SRP is also violated (at least I think it is).

The splitNames method processes input (trims and splits) AND creates a list.
So IMHO the problem of too much knowledge could be solved by splitting the method into 2 others:

One called say String[] processInput(String input) that processes the input string (you never know in what
devious ways you might need to split that string in the future),
 
and another called ArrayList<String> createListFromArray(String[] array) that would create the resulting list.

What do you think?

[Norbert Nemes]

Ok. Since the train wrecks in the name inverter class seemed weird and avoidable,
the refactoring bug got into me and I decided to try to clean up the code.

What I found was the there is no need for the NameInverter class.
All that is required is a PersonName class that knows about names, honorifics and postnominals
and a NameFormatter interface and its implementation, that produces a string based on a
PersonName class.

Here is the code:
-----------------------------------------------------------
PersonName class
-----------------------------------------------------------

import java.util.ArrayList;
import java.util.List;

public class PersonName {
    private List<String> nameElements = new ArrayList<String>();
    private List<String> honorifics = new ArrayList<String>();
    private List<String> postnominals = new ArrayList<String>();
   
    private String honorificRegex = "MR\\.|MRS\\.|MS\\.|HRM|SIR|LORD|MR|MRS|MS|PROF|PROF\\.";
    private String postnominalRegex = "PHD\\.|BS\\.|PHD|BS|MD|CEO|CFO|CIO|I|II|III|ESQ|ESQ\\.|SR\\.|SR|JR\\.|JR";
   
    public PersonName(String name){
        if(name==null) return;
        String[] elements = splitInputName(name);
        for(int i=0;i<elements.length;i++){
            String nameElement = elements[i].trim();
            classify(nameElement);
        }
    }
   
    private String[] splitInputName(String name){
        return name.trim().split("\\s+");
    }
   
    private void classify(String nameElement){
        if(nameElement.toUpperCase().matches(honorificRegex))
            honorifics.add(nameElement);
        else if(nameElement.toUpperCase().matches(postnominalRegex))
            postnominals.add(nameElement);
        else
            nameElements.add(nameElement);
    }
   
    public List<String> getNameElements(){
        return nameElements;
    }
   
    public List<String> getHonorifics(){
        return honorifics;
    }
   
    public List<String> getPostnominals(){
        return postnominals;
    }
}

-----------------------------------------------------------
NameFormatter  interface implementation
-----------------------------------------------------------

import java.util.List;

public class LastFirstPostnominalsFormatter implements NameFormatter {
   
    public String formatName(PersonName name){
        String formattedName="";
        if(name==null) return formattedName;
        formattedName += getLastNameFrom(name);
        String firstName = getCombinedFirstNameFrom(name);
        String postnominals = getPostnominalsFrom(name);
        if(!firstName.equals("")) formattedName+=", " + firstName;
        if(!postnominals.equals("")) formattedName += " " + postnominals;
        return formattedName;
    }
   
    private String getCombinedFirstNameFrom(PersonName name){
        List<String> nameElements = name.getNameElements();
        if(nameElements.size()==0) return "";
        return combineListIntoSpacedString(nameElements.subList(0, nameElements.size()-1));
    }

    private String getLastNameFrom(PersonName name){
        List<String> nameElements = name.getNameElements();
        if(nameElements.size()==0) return "";
        return nameElements.get(nameElements.size()-1);       
    }
   
    private String getPostnominalsFrom(PersonName name){
        return combineListIntoSpacedString(name.getPostnominals());
    }
   
    private String combineListIntoSpacedString(List<String> list){
        String result = "";
        for(int i=0;i<list.size();i++){
            result += list.get(i);
            if(i<list.size()-1)
                result+=" ";
        }
        return result;
    }
}

These two classes pass all the original tests, but can also handle middle names and a large variety of
honorifics and postnominals.
The regexes are kind of ugly, but then again, they always are ;). Also the multiple if - else if statements
in the classify method are bugging me.

So, there you go.

Any feedback would be much appreciated.

[JuanManuel Gimeno Illa]

On Thursday, June 13, 2013 4:10:26 PM UTC+2, Norbert Nemes wrote:

    private void classify(String nameElement){
        if(nameElement.toUpperCase().matches(honorificRegex))
            honorifics.add(nameElement);
        else if(nameElement.toUpperCase().matches(postnominalRegex))
            postnominals.add(nameElement);
        else
            nameElements.add(nameElement);
    }

Any feedback would be much appreciated.

One thing that surprises me of this code is that now the name postnominals does not refer to something that goes after the name (post-nominal) but something that matches some regular expression (without any relation to its position).

Juan Manuel

[Norbert Nemes]

Hello Juan Manuel

Well... not exactly. While technically you are right, that regex will only match words that usually appear
after a persons name. This form was necessary because you never know how many honorifics, and
post-nominals there will be in a name, nor how long the name will be. So you have to scan and classify
each word.

[JuanManuel Gimeno Illa]

Following your idea, maybe an improvement is matching as much as possible honorifics from left to right and as much as possible postnominals from right to left. What remains in the middle would be considered "the name parts".

Juan Manuel

[Norbert Nemes]

I'm not exactly sure.... Think about what it would entail:
You have to have a function that gets the index of the last honorific,
another one that gets the first index of the first postnominal,
and finally a third that splits the name array into honorifics, name and
postnominals (with index validity checking and all).
Plus it leaves the door open for the user to sneak in honorifics and or
post-nominals into the name part (Evil hacker with lots of free time scenario :)).

[JuanManuel Gimeno Illa]

I wouldn't past any index cause each method would return the remaining list to be used for the next parsing step.

List<String> withoutHonorifics = parseHonorifics(elements);

List<String> name = parsePostNominals(withoutHonorifics);

You can pass the list explicitly or share it as an instance variable.

Juan Manuel

[Norbert Nemes]

Oh I see! That is actually a good idea. That would take care of the if-else ugliness and make the code a bit easier to read. What I don't like about it is that it might end up scanning the elements of the input name string almost 2 times (let's say in the extreme case of a Spanish name spanning 2 pages with one honorific and no post-nominals :)). But I guess, you can't win 'em all :). Thanks.