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

Refactor JS #178

Closed
rugk opened this issue Feb 6, 2017 · 2 comments · Fixed by #180
Closed

Refactor JS #178

rugk opened this issue Feb 6, 2017 · 2 comments · Fixed by #180
Assignees

Comments

@rugk
Copy link
Member

rugk commented Feb 6, 2017

I really feel the JS needs to be refactored. @elrido already did something, but it is still not what I'd expect and when working with the JS code I get constantly annoyed about the structure. (There is none. No modules or so…)

So my aim:

  • use this kind of extensible module pattern (it is not really done yet)
  • structure PrivateBin stuff into modules (one for encryption, for UI, etc.). They mus work together though.
    Currently completely unrelated functions are side-by-side in the source code.
  • I think I'd try to leave all the function (parameters) and PHPDOC unchanged (for now). During refactoring I might see when something needs to be changed.
  • Abolish this and use alternatives. It is often unclear what this refers to, so only use this if it is clear. (i.e. clearly do not put all functions into this)
  • Try to make it flexible enough, so one can change the en/decryption routines
  • Edit/Added: Add $ at the beginning of jQuery objects (it's a convention to prevent confusing variables)

Note that I may break the unit tests. I have not looked into them, but I think they are easy to adjust them afterwards provided that the new structure should hopefully be more flexible than the previous one.

@elrido A question first: For what is the $.proxy used?

@rugk rugk added this to the review & refactor paste format milestone Feb 6, 2017
@rugk rugk self-assigned this Feb 6, 2017
@elrido
Copy link
Contributor

elrido commented Feb 7, 2017

$.proxy injects "this" (the second parameter)

For the unit tests to work, all the functions (that you want to be able to test) need to be publicly accessible.

Note on the structure: There are 4 modules helper (generic, independent functions), i18n (functions related to translation, they do share state), filter (wrappers for de/encryption and de/compression) and controller (the big mess that I agree needs to be refactored).

I am working on unit testing helper, filter and i18n functions. Maybe with your refactoring of the controller it will be easier to test that one?

@rugk
Copy link
Member Author

rugk commented Feb 7, 2017

Okay, good to know some basics about the structure, which looks good, but from the first look these modules are not really separated.

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

Successfully merging a pull request may close this issue.

2 participants