Skip to content

Refactor ModuleResult#pre_order#222

Open
rafamanzo wants to merge 1 commit intomasterfrom
refactor_pre_order
Open

Refactor ModuleResult#pre_order#222
rafamanzo wants to merge 1 commit intomasterfrom
refactor_pre_order

Conversation

@rafamanzo
Copy link
Copy Markdown
Member

@rafamanzo rafamanzo commented Jul 22, 2016

This is an optimization made by @danielkza at
805c7c1 which reduces the complexity of
finding the root finding it through the Processing.

I'm not sure the memozation through the attribute @pre_order worked at
and if we should keep it.

@danielkza have a look if you agree with this extraction and @mezuro/core we need a third pair of eyes here as this is a derivative I've made from @danielkza's work.

It is is a slight improvement for #207 that we've to check the impact.

This is an optimization made by @danielkza at
805c7c1 which reduces the complexity of
finding the root finding it through the Processing.

I'm not sure the memozation through the attribute @pre_order worked at
and if we should keep it.
@danielkza
Copy link
Copy Markdown
Contributor

👍 for me

@rafamanzo
Copy link
Copy Markdown
Member Author

After #225 where will pre_order still be used? Maybe is a better investment to refactor what still uses this method in terms the level walking methods introduced there and completely remove this method. What do you think?

@danielkza
Copy link
Copy Markdown
Contributor

That is my plan. As far as I can determine, the only processing step that really cares about navigating the tree from children to parents is the tree builder, but it obviously can't use pre-order as it has to create the tree.

The other steps either don't care about order at all (compound calculator, interpreter, checker), or can work with level-order (aggregator).

I was also thinking: if we were willing to refactor all the steps, we could get away with only fetching results from the database once, after the collecting step. Since only the corresponding job should modify results of a processing before it is done, we can be sure our in-memory representation of the tree is accurate, as long as we create all the new records using build_ methods.

@diegoamc
Copy link
Copy Markdown
Contributor

Needs rebasing.

@rafamanzo
Copy link
Copy Markdown
Member Author

Not sure if we want to merge this or just get rid of pre_order. I'm inclined to the later. What do you think @diegoamc ?

The other uses of this method are Compound calculator and Interpreter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants