Make the code more understandable

[michaeljones]

I have come to accept that the code is unnecessarily complex to understand. I now realise it could be much more readable and approachable. I have defended it in the past but it must be possible to make it better.

I hope that by improving it we might get more contributions from people who are interested but currently turned off by the state of the code. This would definitely help the project as I don't find much time to work on it and move it forward.

Broadly, I will try to reduce the number of factories being used and have less dependency injection. The dependency injection was mostly designed to make the code more testable but I have never written tests to take advantage of that so it is just a cost to readability at the moment.

I would love some input to help identify particularly difficult parts of the code or to propose improvements. I am keen to improve it now but not entirely sure how to start. But I will try :)

[vitaut]

For me the most difficult part was DoxygenToRstRendererFactoryCreatorConstructor. I am still not sure I understand how it works =):

    math_nodes = collections.namedtuple("MathNodes", ["displaymath"])
    math_nodes.displaymath = sphinx.ext.mathbase.displaymath
    node_factory = NodeFactory(docutils.nodes, sphinx.addnodes, math_nodes)

    rst_content_creator = RstContentCreator(ViewList, textwrap.dedent)
    renderer_factory_creator_constructor = DoxygenToRstRendererFactoryCreatorConstructor(
        node_factory,
        parser_factory,
        DomainDirectiveFactory,
        rst_content_creator
        )

[michaeljones]

Ha, ridiculous name eh? I am very happy to try to get rid of it. Though that class itself does very little so is it more than whole DoxygenToRstRenderer stack that you'd like to see improved or you just want to be free of that one class?

[vitaut]

Name kind of makes sense if you are familiar with musical terminology like hemidemisemiquavers =). I'd go with simplifying this chain of factories first if possible. Currently it looks like all the callers use the same pattern:

        renderer_factory_creator = self.renderer_factory_creator_constructor.create_factory_creator(
            project_info,
            self.state.document,
            target_handler
            )

        try:
            renderer_factory = renderer_factory_creator.create_factory(
                node_stack,
                self.state,
                self.state.document,
                filter_,
                target_handler,
                )
        except ParserError as e:
            return format_parser_error("doxygenclass", e.error, e.filename, self.state,
                                       self.lineno, True)
        except FileIOError as e:
            return format_parser_error("doxygenclass", e.error, e.filename, self.state, self.lineno)

        context = RenderContext(node_stack, mask_factory, directive_args)
        object_renderer = renderer_factory.create_renderer(context)

Since all three levels of creation happen together, perhaps this chain of factories can be collapsed.

[michaeljones]

That's interesting. I'll give it some thought. I've made some progress getting rid of some stuff. I'll push in a bit I hope.

[vitaut]

@michaeljones, great job with refactoring! I have another idea that I want to discuss with you before starting any development.

The idea is to apply the visitor pattern to renderers. To be more specific, I suggest introducing a Doxygen node visitor class:

class DoxygenVisitor:
  def visit_function(self, node):
    pass # called on memberdef node of function kind

  def visit_typedef(self, node):
    pass # called on memberdef node of typedef kind

  ...

and using it to implement the Sphinx/RST renderer:

class SphinxRenderer(DoxygenVisitor):
  def visit_function(self, node):
    # renders Doxygen function node using Sphinx function directive from the current domain

  def visit_typedef(self, node):
    ...

I think this will simplify the code because all the shared state that currently needs to be passed through *Renderer objects will reside in a single place, SphinxRenderer. In fact, the node to render seems to be the only non-shared state and it will be passed as an argument to visit methods.

DoxygenToRstRendererFactory will become unnecessary (although some of it code will probably move to DoxygenVisitor) because there will be one renderer class. At the same time it will still be possible to modify the rendering behavior by subclassing SphinxRenderer and overriding its methods.

This could also improve performance because we won't be creating many *Renderer objects.

What do you think?

[michaeljones]

That sounds really promising. I'd be really interested to see that. I'm happy with the little progress I made trying to cull stuff and clean up a bit, but the code is so familiar and such a representation of how I've been thinking about Breathe throughout the project that I really struggle to imagine fundamentally different ways to approach it. We definitely need to shake it up a bit to have a better go at making it a more approachable code base.

Please go for it and let me know if I can help. Though I suspect such things are tricky to share. Happy to help if I can though.

[vermeeren]

I think it's safe to say that refactoring is completed at this point.

I do agree the code should still be made more understandable, that will be handled in #357.

Closing this for now as duplicate of #357.