[Spice-devel] Refactory and spaghetti code

[Frediano Ziglio]

Cfr https://en.wikipedia.org/wiki/Spaghetti and
https://en.wikipedia.org/wiki/Spaghetti_code!

As today part of the team involved in the refactory review is on holiday I
started looking at the average direction and state of this refactory.

I looked at different angles like:
1- where we are;
2- what are actually doing;
3- what is the status of final code, looking at last commit in the branch.

Honestly I'm not that happy on the status.

1- we just started the refactory. The reason is that code is quite old,
there are plenty of dependency here and there, the code is mainly in a
single file hard to understand full of small/medium/large functions.

2- actually we are trying to split this huge amount of code into separate
files each handling a specific piece of functionality. The branch could be
split in 3 pieces:
 2.1- split into functionality files (actual one, about 150 patches)
 2.2- remove global states;
 2.3- encapsulate in gobject/glib.
After 2.1 should be easier to understand where to edit a functionality and
as dependencies should be less easier to add new ones or fix them.  I
repeat... SHOULD. If this were wrong after 2.3 code should not look that
much as spaghetti code!  Don't misunderstand me, the final code is better
but IMHO far from what we want to probably achieve.

I think that when a refactory is needed you should better understand the
structure/features/objects which are involved in order to better define the
relationship between them. I think this point is a bit missing. Let me make
an example of what I mean. What a RedWorker should be supposed to do? What
is it? IMHO a RedWorker should handle a thread that manage messages between
display and cursor channel. Easy and simple definition but it comes with
lot of implications:
- thread informations are RedWorker stuff;
- should deal (receive and send) messages dispatching to display and cursor
  channels. Once messages is dispatched the job is done! This means that
  RedWorker should not handle any message itself!
- should not deal with drawing object as DisplayChannel will do it. This is
  the reason a lot of code is currently been moved from red_worker.c to
  display-channel.c/stream.c/others. But in the final commit there are still
  some fields which are strictly related to the display :(

3- oh my god! Try to open red_worker.c (and many other files) and see the
FIXME, TODO and similar. Plenty of commented out code which were removed as
too difficult to implement (and are working in the current master!). Well..
still to review/improve but surely not the code we want! The reason? Code
is still in red_worker but would be better moved to other files. Note that
separation was meant to be mostly completed after step 1 so the fact that
after step 3 code is still there means that step 1 was not really
completed.

One way to check for dependency in C programs are to see:
- function calls between modules (objdump or similar tools could be
  helpful);
- include dependencies.
Another thing to look are accessors functions (get/set).

Let me show some example of what I found (take a look at final commit in
refactory branch):
- red_drawable_count in RedWorker. This field count drawables allocate in
  this worker. The field is used in red_drawable_unref and red_drawable_new
  (and some commented out code!). The RedDrawable structure is defined in
  red_parse_qxl.h. The life of this RedDrawable object is split between
  red_parse_qxl and red_worker creating a strong dependency between them.
  Note that red_parse_qxl mainly deal with reading messages from VM and
  managing structures at low level and has only an exception for this
  RedDrawable. The only thing is able to do is incrementing the reference
  counter while freeing and allocation is demanded to red_worker. Moving the
  refs to an upper layer structure would solve the issue. Another comment on
  this is why we have to keep Drawable and RedDrawable? Why don't
  DisplayChannel allocate a Drawable from RedDrawable and use it instead of
  dealing with two objects?
- red_process_draw/destroy_primary_surface, mainly a bunch of calls to
  DisplayChannel
- red_migrate_display, why still in red_worker?
- red_worker_get_cursor_channel/red_worker_get_display_channel. These are
  called to finally initialize cursor/display channel allocated in
  red_worker_new. So red_worker_new partially initialize itself, then returns
  to red_dispatcher_init which is expected to finish the initialization of
  these structures. Note that part of the initialization is set some callback
  to some static functions defined in red_displatcher.c and strictly
  associated to cursor/display channel. Now just to demonstate how not that
  maintanable is that code try to add a function to destroy RedWorker and see
  how many files/functions you have to edit.

I think that after a *_new function a a *_destroy *_unref could be called
straight away. Actually this is not true. The reason why this should be
true is that if some some reason initialization fails you should free the
object. The reason why this is not currently true is that in some cases the
*_new function allocate but not initialize entirely and another function is
demanded to initliaze the object to a correct state (and at this point is
possible to call *_destroy/*_unref).

Frediano