Episode 19 - Advanced TDD Part 1 - Mistakes on refactoring

[nabil ouachi]

Hi Bob,

I have a question about the "NameInverter" class, you don't think that this class do a lot of things and its violating the Single Responsibility Principles. For example, if we wont to change the format of the returning String: We don't wont anymore to return the name in this format : "Last First", instead we like this one: "Last - First". This, lead us to make change to the NameInverter class, thus, we have to recompile and rebuild this class.

Is that one of the mistake you talked about at the end of the episode ?

Thanks,

[Uncle Bob]

Nabil,

Hmmm.  Perhaps.  The Name Inverter performs a formatting operation.  It takes a name in a standard format and changes that format.  Therefore it is not surprising that a change to the formatting requirements would cause a change in the NameInverter.

However, you _could_ break the NameInverter up into two different parts.  The first would rearrange the elements of the name into the new order, and the second would format those rearranged names.  Those two concepts could be separated into different classes so that ordering changes and format changes could be kept separate from each other.

I would do this only if formatting and ordering were changing for very different reasons (i.e. one group of stakeholders worried about ordering, and another worried about formatting).  

[nabil ouachi]

Ok, and I think you do this to avoid Needless complexity, since no body ask for change to the formatting requirement, we keep it simple as it is, the story of: Fool me once, shame on you, Fool me twice, shame on me ;). 

Thanks Uncle bob,

[Andreas Schaefer]

if the NameInverter class is in a seperate component (e.g. package, assembly) that is losely coupled, if the requirements change, you'd be able to just replace this one component without having the need to recompile other parts of the system.

[milton go]

Hi Bob,

I have a question regarding the "NameInverter" class, it feels weird that no variables where extracted from the class. Is this one of the mistakes that you mentioned? Shouldn't a class at least have a common variable/field that is used by the class? 

[Marco Tedone]

I hope the green bands are still available :-)

I made the class public and moved it to the "src/main" part of the code. I made all methods, except invertName private and I made the invertName method public. 

Also, and this is not something that was missed, but rather a suggestion for improvement, I've implemented the postNominals method with String.join, as follows: 

```

private String extractPostNominals(List<String> postNominals) {
return String.join(" ", postNominals);
}

```

Additionally, I felt I didn't like the removeHonorifics to return the same list which changed state, so I thought of applying (perhaps unnecessarily) a bit of defensive programming: 

```

private List<String> removeHonorifics(List<String> names) {
if (names.size() > 1 && hasHonorific(names.get(0)))
names.remove(0);
return new ArrayList<>(names);
}

```