[Kstars-devel] Refactoring of SkyComponents

[Khudyakov Alexey]

В сообщении от 30 января 2010 23:29:11 Akarsh Simha написал:
> Hi Alexey,
> 
> Sorry to barge in late, but I have a few questions just for my
>  understanding.
> 
Ah that's nothing

> > * Unused functions and data members are removed. I used rather strict
> > approach to removal of unused functions. If function is not used it is to
> > be removed. This is especially true for accessors.
> 
> Why is this a good idea?
> 
1. Unused functions are likely to have bugs. Especially if they haven't been 
used for a long time. 

2. They clutter class interface. Class with 10 public functions isn't same as 
class with 7 public functions.

3. They make class harder to modify. One have to preserve their functionality 
and if he fails it won't be revealed. They are not used after all. 

4. Accessors in particular are very cheap to write if necessity arises and 
expose inner structure of class needlessly.

Also there were nothing of real value. They are mostly remnants of old 
changes.


> > * If class A is only subclass of class B merge them into one class.
> > I think it's better to have fewer classes. And it's reasonable to keep
> >  - SkipListIndex -> MilkyWay
> >  - SingleComponent -> SolarSystemSingleComponent
> 
> Why is this a good idea? Isn't it a better idea to have things like
> SkipListIndex and SingleComponent hanging around, because we may later
> want to add other subclasses?
> 
Indeed this is clear tradeoff. Unified class is easier to understand and tweak 
but it's problematic to extend it. On other hand two class design is easier to 
extend but more difficult to maintain. I considered that possibility that 
another class would be added. There is another consideration. When one change 
class interface it's clearly advantageous to have fewer class. It's easier to 
modify N-1 class than N.

Case of Milky way is special. After refactoring in become SkipListIndex with 
special constructor and drawing function. Another problem is awful design of 
SkipListIndex. It uses blind casts of LineList* to SkipList* and will fail if 
used improperly. I don't know what does "properly" mean in this context. It 
ought to be fixed. Such fix could bring major changes to that corner of class 
hierarchy and I think it would be better to have fewer classes.