When studying the code of the 2 mentioned files, several questions/remarks/ideas arised.
1) looking at the header : why the protected part, it seems they may be private. Stroustrup says that protected members are evil (protected method are less evil).
Argument pro : maybe somebody 'might' derive from it, but then it would be better to make the destructor virtual.
Several get functions are not const,m we'll see later why. Getters are adviced to be const methods. Derive interfaces, aggregation is advised for code reusion (inheritance is very strong coupling).
Below are some code lines from the cpp file
void PersonalityManager::ReadPersonalities()
{
m_Personalities.Clear();
m_Personalities.Add(_("Full IDE (default)"));
}
const wxString& PersonalityManager::GetPersonality()
{
static wxString pers;
pers = m_CurrentPersonalityIdx > 0 && m_CurrentPersonalityIdx < (int)m_Personalities.GetCount()
? m_Personalities[m_CurrentPersonalityIdx]
: wxT("default");
return pers;
}
const wxArrayString& PersonalityManager::GetPersonalitiesList()
{
return m_Personalities;
}
const wxString& PersonalityManager::GetPersonalitiesRoot()
{
static wxString root = _T("/personalities");
return root;
}
const wxString& PersonalityManager::GetPersonalityKey()
{
static wxString key;
key = m_CurrentPersonalityIdx <= 0
? wxT("") // default personality
: GetPersonalitiesRoot() + _T("/") + GetPersonality();
return key;
}
2) 3 different ways for getting in the text
a) _("Full IDE (default)")
b) wxT("default")
c) _T("/personalities")
--> maybe it is better to make it consistent ? What is the preferred way of those 3 ?
3) GetPersonalitiesRoot() is not const :
- why because it fills in a static (non const) variable --> bad : is like a global variable, but with some more scope restriction
- Suggestion : use a static const in the class body
static const wxString root = _("/personalities");
then we can make the Get const :
const wxString& PersonalityManager::GetPersonalitiesRoot() const
{
return root;
}
-> note this can become a static method
4) GetPersonalityList : --> can be const method
5) GetPersonality method is not const
a) is not consistent in the case no personalities : shoudl be again "Full IDE (default)", and that one should be present (added in the constructor) , so no need to create a static variable here, just return a const reference into the m_Personalities array --> const method
b) besides even if a new value should be needed to created on the fly, it is better to construct it in the return statement , so the compiler can apply the 'optimize return value' idiom (see Meyers' books).
c) in this case we could have worked with return by value (though wouldn't do it, since this is not a function called that often (startup))
d) is not reentrant safe, say 2 CB instances nearly arrive at the same time in this function, and 1 looses cpu to the other just before the 'return pers;' line, then the other will override the value, and when cpu return to the first one, it's returning not the intended value !! (non const) Static's are evil !
I have the impression they were static so they live after the function return and one could return a reference to it. Maybe there are other good reasons, can't think of one at this moment, but hey, willing to learn anytime.
6) GetPersonalityKey is not const => nearly the same as in 5)
- might be better to return in case the index <= 0 (should not happen ? Full Ide(default) ?) root/Full IDE (default)
7) maybe add : static const member :
static const wxString m_Default = _T("Full IDE (default)");
can then be used to return the default, or document that at index 0 , the default lives.
Cheers,
Lieven
