search

OpenUserJS / OpenUserJS.org

The home of FOSS user scripts.
https://openuserjs.org/
GNU General Public License v3.0
847 stars 301 forks source link

Remove redundant legacy code and improve overall consistency #262

Open sizzlemctwizzle opened 10 years ago

sizzlemctwizzle commented 10 years ago

My haphazard design decisions during initial development combined with the enhancements of @Zren have left the code base full of redundancies and inconsistencies. The current state of the code is borderline incoherent. I doubt anyone other than @Zren and myself have any clue how things are structured.

The solution is to remove all redundant legacy code and to make all existing code conform to the same pattern. This includes finishing the discussion software since its lack of the ability to be voted, flagged, and removed is inconsistent. The voting and flagging routines need to be modified to match removal (since that was the last one I implemented, it's the most well formed). These routes will be converted to POST routes and accessed via Ajax on the client-side.

This issue needs to be resolved before we can move forward so added the blocking label (also I'll be changing things all over the code base and we don't want to run into merge conflicts). Please refer to our contributing documentation for specific information on the meaning of that label.

Although I've assigned myself to this issue, I'll be pushing a branch to this repo called issue-262. If anyone wants to help out, just mirror this branch on your own repo and let me know about any changes you want me to pull in the comments here.

Martii commented 10 years ago

Agreed... I'm lost. I have no clue how to do certain things. I have no problems redoing any snippets I've done but it's really difficult to figure out what to use and where. Part of my skill set is reverse engineering but I'm at a loss at this time.

Applies to:

sizzlemctwizzle commented 10 years ago

it's really difficult to figure out what to use and where.

Definitely. Even I struggle sometimes, which says something about the current state of affairs.

Martii commented 10 years ago

If I could make one request... when declaring function arguments... please use aIdentifierName the a indicates that it's part of an argument in a function.. that makes code inherently easier to read (including without having to scroll all the way up to the function declaration and then back down) ... also allows intermediates to be defined if we need them with a non-a-prefixed identifier name. Doesn't have to be all at once if you don't want to but it sure would help.

sizzlemctwizzle commented 10 years ago

That sounds good. I also plan on doing some rough documentation of the design as I go through the code. I'll put it in our wiki. But don't expect something grand since I'm focusing on the code.

Martii commented 10 years ago

Well documentation is a forte of mine so I can wiki assist for sure.... even if it's something as simple/similar as $ npm ls output to show our objects and methods/properties I could at least figure out what to use and where.

jerone commented 10 years ago

+1 on the global idea. Will there be a list of things that requires addressing? Can I also suggest using more of the Promises pattern (Promises/A+ support?).

Martii commented 10 years ago

That's a good idea @jerone but I also usually see a lot of promises not fulfilled in my browser console quite often, even from Add-ons, and not sure if we want to do this at this stage of a refactor... IDK for sure.... also does V8 via node.js support these? They are relatively new.

sizzlemctwizzle commented 10 years ago

Can I also suggest using more of the Promises pattern

I hate promises. They're a sloppy way to deal with asynchronous code imo. I prefer the async library. We just need to make sure that all callback functions conform to the (err, results) pattern (with both being either an object or null), which some don't atm.

Will there be a list of things that requires addressing?

I've created a milestone for this blocking issue, and I know there are several other related issues can be attached to this milestone as well. You can propose others that don't already exist if you feel they fit within the goals of this issue. I'll probably create some of my own as I encounter things that need discussion.

I'll begin my initial work (planning) tonight and report my progress. I seem to do my best work when the sun goes down.

Martii commented 10 years ago

I seem to do my best work when the sun goes down.

hehe me too.

Zren commented 10 years ago

If anything, removal should confirm to flagging rather than the other way around. That way it's just a boolean on off to undo a removal.

sizzlemctwizzle commented 10 years ago

I decided that rather than just jumping in head first tonight, I'd do some planning. So far this is my plan of attack:

1. Remove all legacy routes, controllers, and templates (Done) 2. Add comments to app.js documenting the file path of the controllers for routes. Yes it's obvious in most cases, but it would be nice not to have to look at the requires at the top of the file. (Will do some other time)

  1. Identify misplaced controllers (the ones dealing with scripts in controllers/user.js jump to mind) and move them to the appropriate file
  2. Normalize vote/flag/remove as POST routes. This will require the most work since only "remove" is well ordered (although it could be improved). These will all have to be modified to work on discussions and comments but that should be easier once they've all been properly abstracted. They'll all probably share a library in common because of their similarities and just extend it with their differences. 5. Normalize all controllers by identifying common logic to reduce redundancies and inconsistencies, mostly by using modelQuery and modelParser. But I might have to break those up into their model-specific parts so they're easier to navigate (don't worry, I'll use a subfolder for each). (I've decided to leave this to another issue/someone else)
  3. Finally update the views (mostly discussion) and controllers for the changes made on the server-side.

Edit: List updated on Oct. 2nd and I'm currently working on #4 above.

jerone commented 10 years ago

Another one; clean up the remainings of the old ui (logic and content like images).

Martii commented 10 years ago

@sizzlemctwizzle Did you want me to zip (probably too strong of word heh) through the source to normalize the parameter lists to prefixes of aCamelCasing while you are pondering? ... mundane tasks like these I can do with good accuracy... I'm used to it by now.

EDIT: Btw is this branch on my repo something that I can/should get rid of? It seems to be orphaned in local but I don't know for sure because it came with initial cloning of upstream here.

sizzlemctwizzle commented 10 years ago

Sure. That'd be great. On Jul 14, 2014 2:19 PM, "Marti Martz" notifications@github.com wrote:

@sizzlemctwizzle https://github.com/sizzlemctwizzle Did you want me to zip through the source to normalize the parameter lists to prefixes of aCamelCasing while you are pondering?

— Reply to this email directly or view it on GitHub https://github.com/OpenUserJs/OpenUserJS.org/issues/262#issuecomment-48946776 .

sizzlemctwizzle commented 10 years ago

I just merged recent PRs along with my removal of legacy stuff. These changes have been deployed so let me know if anything is broken on the site in case I missed something during testing.

Martii commented 10 years ago

Flagging a User doesn't work any more... returns a 404.

jerone commented 10 years ago

@sizzlemctwizzle commented on 13 jul. 2014 23:17 CEST:

...The solution is to remove all redundant legacy code and to make all existing code conform to the same pattern...

Is this still work in progress? Still hoping this is the case.

PR's with bug-fixes and new features are being added to the current code while the label BLOCKING is still active.

Martii commented 10 years ago

PR's with bug-fixes and new features

That PR, a tool for debugging purposes only, is walking the line just like yours did with that boog (otherwise it would have sat unmerged and undeployed). We have quite a few boogs with our dependencies as I have shown on the README.md. If you read BLOCKING too it does allow for bug fixes. If you are in the area and find a bug and the related enhancement isn't too over the top go for it in a PR.

Is this still work in progress?

Yes he is working on it. Things take time. It took me two months to develop and test just the JSON for installWith. :)

sizzlemctwizzle commented 9 years ago

I'm still working on this (been busy lately but I'll keep at it). I've updated my progress here. I've removed the blocking label since I've decided to focus mainly on the vote/flag/remove functionality and adding those to discussions. I've removed the blocking label since my focus has become more isolated and I should be able resolve collisions with other PRs. Overall just please stay away from vote/flag/remove for now. You can think of that code as blocked. I just don't want to block everything to block that.

jerone commented 9 years ago

So this issue has been going on for a few months now (since July), and I would like to know the state of it.

This big overhaul seems like a good addition to this repository, but it's blocking any input from developers. It made me stop and wait for this to merge.

@sizzlemctwizzle Can you tell us here how for you're to get this done. Or at least part of it done. Do you need help? Can we do something? Keep us updated in a public way.