On Sat, Aug 18, 2012 at 9:37 AM, Michael Vehrs <[email protected]>wrote:
> On 17.08.2012 16:56, Ola Sundell wrote:
>
>>
>> I've been peeking at the code base and I ran a Sonar run just to do a
>> health check. Thus far, this is what I've seen:
>>
>> Good Things(tm):
>>
>> * short but sweet methods
>> * pretty well-structured packages
>> * compiling works out of the box
>> * you've avoided Maven (hooray!)
>> * quite a bit of javadoc for the public methods
>>
>> Smells:
>>
>> * Huuuge classes (InGameController is 3866 lines! Yikes!!!)
>>
>
> I don't think line count alone is indicative of a problem. If the class
> can be split into several in a sensible manner, by all means go ahead, but
> I don't see any obvious fault lines. Some helper methods could be moved
> elsewhere, but I don't really see the benefit.
I'm not proposing huge swooping refactorings. I'm proposing smaller,
step-by-step refactorings where you, for instance, might pull high score
handling out of the FreeColServer class. As I mentioned, I really think
that the code base is sound and in very good shape but for the size of the
classes. Large classes makes the code hard to digest and understand. It is
hard to avoid code entanglement. It is also (usually) hard to write decent
tests, and as the saying goes, if the code is hard to test it's usually
time to refactor.
Personally, I am a fan of delegation. For instance, I won't argue whether
it is a good or bad idea to let the Unit class be able to (de)serialise
itself or whether you should let a separate Factory class handle
deserialisation and another for serialisation. However, I'm of the opinion
that the -actual code- needs to be in the aforementioned class. Let the
public methods remain in Unit as one liners which delegates work to a
UnitSeralizerDelegator (or something like that) with package local methods,
thereby cutting down the size of the the Unit class.
* Old-ish versions of external libraries
> * Code coverage could be a little bit better
>
Definitely. New unit tests are always welcome. The GUI, in particular, is
> in need of some automated testing, but this is notoriously difficult.
>
Indeed. I have done quite a bit of work when it comes to automated GUI
testing, mainly for dynamical websites, but also for AWT/Swing. It is, of
course, hard to write relevant tests, especially tests that actually test
your own code - lots of tests I've seen test AWT and Swing stuff...like,
"oh, new JLabel("foo") created a JLabel instance this time as well! Yay!"
I hope our lack of enthusiasm hasn't offended you. We certainly could do
> with more contributors, but bug fixes, unit tests, new features,
> small-scale code cleanup and improved documentation would be more welcome
> than any large-scale refactoring.
>
As I mentioned earlier, I am not proposing large-scale refactorings, I am
proposing a slow and steady approach to make the big classes smaller,
thereby creating more coherent pieces of code which are easier to unit
test. I am also not in the least offended by your supposed lack of
enthusiasm - on the contrary, I am the new guy without any history in this
project, telling you to do stuff differently. If anyone should be
considered offensive, it's me.
I do not see this as a lack of enthusiasm, either. I would interpret a
"yeah, whatever"-response as lack of enthusiasm. A "don't do this, please
do that instead"-response, like the three responses I've recieved thus far,
is a clear indicator that you are all pretty enthustiatic... ;-)
Ola
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Freecol-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/freecol-developers
