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

Intimidating code... #26

Open
gdborton opened this issue Mar 13, 2017 · 3 comments
Open

Intimidating code... #26

gdborton opened this issue Mar 13, 2017 · 3 comments

Comments

@gdborton
Copy link
Contributor

While digging into #24, it took me a long time to understand what was going on in the codebase, and then took me even longer to understand why.

After trying to grok the code for longer than I'd like to admit, I can see that...

This code:

return setThisDescription(concatThis(this, newWrappers), description);

is essentially a different way of doing this:

return new MochaWrapper({
  wrappers: newWrappers,
  description,
});

And that because of attempts to maintain privacy this...

return someChainableFunction(someOtherChainableFunction(myInstance));

was basically the same as this...

return this.someOtherChainableFunction().someChainableFunction();

My question is... would you be ok with:

  1. Changing the functional code to be more object oriented (we're passing instance a lot)
  2. Removing the attempts at maintaining privacy

^ I think that doing these would help to make the code much more approachable.

We could be explicit in that the internal functions/parameters are not part of the api, and will break without breaking changes.

@ljharb
Copy link
Collaborator

ljharb commented Mar 13, 2017

Everything reachable is part of the public API - I'm very concerned with maintaining privacy.

I'd be fine with any changes that increase readability without exposing any additional API surface though!

@gdborton
Copy link
Contributor Author

Would your definition of reachable include additional files? If not, we could create a PrivateMochaWrapper with all the internals chainable + easily readable, and then change the public api functions to return instanceOfPrivateMochaWrapper.someLogic().getPublicWrapper().

This could also be done in a single file, but I'd prefer to separate out the private/public version for organization.

@ljharb
Copy link
Collaborator

ljharb commented Mar 14, 2017

Technically yes, additional files would be included. This can be avoided by bundling on prepublish, and npmignoring the unbundled files, but I'd planned to avoid a build process in this module.

However, even in separate files, if instanceOfPrivateMochaWrapper is obtainable, then encapsulation is not achieved - if it's not, then perhaps there's a refactor that could be done in the same file, to make the code a bit more readable while not yet tackling the issue of "separate files"?

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

No branches or pull requests

2 participants