Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code refactoring #117

Open
jaydson opened this issue Feb 10, 2015 · 6 comments
Open

Code refactoring #117

jaydson opened this issue Feb 10, 2015 · 6 comments

Comments

@jaydson
Copy link
Contributor

jaydson commented Feb 10, 2015

It's time to apply some serious code refactoring on Harmonic.
The project has evolved and we discovered a lot of quirks in tools we're using and within our own code.
As a bleeding-edge project, this kind of thing can happen at any time, and this is awesome IMHO.

I'm labelling this issue with 'discussion' and 'p2' because we need to define some goals first, but after that, refactoring is absolutely one of ours priorities.

I'm starting to draft a model(kind of) of what we can do in our code structure, based on what we already have:
harmonic

What we have in this first draft:

  • ./bin folder cleanup
  • dedicated cli, config, resources folders. The idea here is to isolate specific code inside those folders
  • skeleton and client folders moved to resources folder
  • new folder core_modules*

core_modules folder:
All internal modules must to be placed inside the core_modules folder.
We can split the code in tiny modules, so we can re-use in CLI, API, etc.
Another benefit of the core_modules folder is that we can identify future node modules, so we can "promote" them to npm.

I definitely need some help and feedback here @UltCombo @leobalter @felipenmoura

@UltCombo
Copy link
Member

Very nice start! 👍

We could focus on the API, then the CLI would just have to call API functions behind the scenes. This way we don't duplicate code and it eases keeping feature parity betwen API and CLI.

Other things we should clean up:

  • Passing sitePath around: currently, most if not all functions in parser.js receives sitePath as argument (this is kind of a hack I implemented to fix tests/allow more flexibility). We could transform parser.js into a central harmonic.js which exposes a Harmonic class -- e.g. it could be the main API endpoint which takes sitePath as a constructor argument and stores it into a "private" property (undocumented, with a leading or trailing underscore). Of course, most of the current parser.js file can be broken down into smaller pieces and placed in the core_modules dir.
  • Globals: we should avoid globals in order to provide a good API. Once we have a Harmonic class, I believe we can just move the globals to properties of the Harmonic instances.

Misc:

  • Perhaps rename core_modules to lib? This is the convention I'm used to see, but I guess this doesn't matter much.

@felipenmoura
Copy link

Really cool.

I believe it would also be cool to have some hooks, like "before building", or for each generated file.
For instance, I would want to get a list of videos from a feed(from my youtube channel) to bind onto a template.

@jaydson
Copy link
Contributor Author

jaydson commented Feb 10, 2015

Great insights here. Thanks both @UltCombo and @felipenmoura.
We can change the core_modules folder name, it was just an example.

@UltCombo
Copy link
Member

@felipenmoura I believe that fits in #6

@UltCombo
Copy link
Member

@jaydson I think we should do this in small (but "complete") steps. E.g.:

  • Refactor directory structure
  • Break down parser.js into core modules
  • Develop API
  • Refactor CLI to use API calls

We can change the core_modules folder name, it was just an example.

I don't really care about the name either, it was just a random idea. 😛

@UltCombo
Copy link
Member

UltCombo commented Apr 2, 2015

Can we focus on this refactoring soon? I guess it would be nice to get this done as soon as we merge the currently open big PRs/branches (ESLint, external themes).

@jaydson can we schedule a hangout to kick off this refactoring?

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

No branches or pull requests

3 participants