Closed jjravleuven closed 8 years ago
jjravleuven
commented
8 years ago Basically this needs to be rewritten from scratch. I'm going to rewrite it. it is disturbing to me that people write code - don't test it, clean it and make sure it is 100% error free - before releasing it to the public.
michaelbromley
commented
8 years ago Hi, Thanks for bringing this to my attention. However, I have a number of problems with this issue:
Linters are mostly a stylistic & heuristic tool. That is, you can have perfectly valid code which may still produce linter errors (as is the case here). For example: 'dirPaginateDirective' was used before it was defined - due to the way that functions are hoisted in JavaScript, it is perfectly valid to do this, and is indeed the recommended pattern in the most widely-used AngularJS styleguide. Likewise, there are indeed times when it makes sense to use != rather than !== - making that call depends on a good understanding of the code involved - something the linter cannot always judge, which is why they usually provide switches to turn off certain inapplicable checks.
"it is disturbing to me that people write code - don't test it, clean it and make sure it is 100% error free - before releasing it to the public.".
Linting is completely different to testing, and serves a different purpose. This module actually has a pretty huge test suite - over 1000 lines of code vs. ~500 for the module itself. I would say it is unusually well-tested.
If you managed to find some code - public or otherwise - that is definitely 100% error free, I'd be very interested to know.
"Basically this needs to be rewritten from scratch. I'm going to rewrite it."
A complete rewrite is a drastic action to take when there are just some linter errors which don't actually impact the working of the code itself. But feel free - I'll happily review a pull request.
At the very least, I fully agree that parseInt() should be used with a radix parameter. My bad for missing that. If you want to add a linter to the build pipeline, that would also be constructive.
jjravleuven
commented
8 years ago If you managed to find some code - public or otherwise - that is definitely 100% error free, I'd be very interested to know.
In all the code which I write, and I have migrated to AngularJS exclusively - I have - within all of my code - zero warnings
It is possible to write code which is pristine. It just takes a bit more due diligence.
I realize that Javascript is a very forgiving language, but I do also believe that due to the number of new developers whom have entered the marketplace - whose focus is entirely on release and not quality - the quality of the work within our industry has dropped exponentially.
I've also - because I was curious - run a few tests and linted the code within AngularJS and the entire framework has less than 1% error in code based upon what you've written.
I mean no disrespect but linting is always a good place to start prior to serious code testing because it eliminates the obvious. I run all of my code through lint testing before I run code testing on it, and there is great satisfaction in "0 errors 0 warnings" in all the code I write.
There is also great satisfaction within the reality of knowing that my code - can stand up against all tests given it. No one ever suffered by aspiring to be better - and no one ever improved by dismissing their own errors, with excuses, justifications, validations, or flippant arguments.
michaelbromley
commented
8 years ago I agree with you that linting is a good thing. I have no argument with the concept. I agree that it would be better if this project used a linter in the build.
I do argue, however, that achieving a 100% pass from a linter gives no guarantee of the logical correctness of your code. Your 100% error-free code is only error-free as far as the linter is concerned. Whether it functions correctly is another matter.
Unit tests are a much better guarantee of this. Still not perfect, but nothing is.
There is also great satisfaction within the reality of knowing that my code - can stand up against all tests given it.
Agreed. Linting is just one type of test you can use against your code. Personally I get more satisfaction from a comprehensive unit test suite passing than from a linter's output. Ideally both are present.
I mean no disrespect
Then telling me that "Basically this needs to be rewritten from scratch." was probably not the way to put it originally.
jjravleuven
commented
8 years ago heh - of course my code functions properly. I use jasmine, built within an full grunt environment and e2e testing in a live coding environment etc - I have 25+ years experience in a wide array of languages if I was inadequate it would have been told by this point.
Then after ensuring my code functions properly, then and only then do I run it through a linter - to clean it up and then again, a fully verbose compile test - again to read the entire flow - and then again - testing it online on various testing environments to see if there are any thing I may have missed.
Now in regards to your pagination "code works properly" i've found numerous bugs within it, which I've had to modify. So it doesn't work correctly. It doesn't use an existing scope, rather it creates an entirely new scope with the elements from the existing scope - and that is duplication within angular - and as you know good angular design is to keep an eye on the size of the scope properly creating and destroying what isn't used.
The searching doesn't work alphabetically either, because I tested it with a generic alphabetical search and even after forcing the pagination code to use an existing scope rather than create entirely new scope assets, it searches from nested array items, rather than simply the represented scope items.
The pagination is counting incorrectly and entire functions are disconnected and not called at all, isolating them from the code.
You know this - i would be surprised that you didn't know it.
i'm not going to fork this repo. I am however going to borrow the same elements you borrowed from Google and write a completely different one, but as it's not my nature to make it public and to customize it for the individual project - I suspect this conversation is over.
Ego has no place in our industry, because it is fully and wholly rendered in the quality of the code.
It is also interesting to me, that you haven't even made an effort to read through the entire area's of error which do not all come from JSLint.
I want to make that clear.
michaelbromley
commented
8 years ago I suspect this conversation is over.
:+1: :100: :white_check_mark:
jjravleuven
commented
8 years ago Ok here's the thing and the reason i have to rewrite your pagination script from scratch.
1: very rarely does one use an object of:
$scope.items[
item: {
name: "Jane Doe",
age: 21
},
item: {
name: "John Smith"
age: 25
}
];In most scenarios the $scope.object is:
$scope.items[
0: {
id: 1,
user: {
name: "Jane Doe",
age: 21
},
department: {
location: "123 Street",
position: {
input: "select",
selected: {
id: 3,
value: "HR"
}
}
},
created: <date>
},
1: {
id: 2,
user: {
name: "John Smith",
age: 25
},
department: {
location: "456 Avenue",
position: {
input: "select",
selected: {
id: 2,
value: "Shipping/Receiving"
}
}
},
created: <date>
}
];Your pagination fails in this scenario. Yes it will list the items, yes it will (if properly configured) render the objects. However, the more in-depth the code is written (not using partials for example and creating html elements on the fly dynamically) then the paging, searching fails.
Instantly the $index is re-ordered - because each of these scope elements are re-created within the scope and isolated from the initial scope, rather than simply accessing the existing $scope.items object - which is poor angular $scope management. Why would you duplicate an existing scope into new individual scope items when they already exist? It's not an unreasonable question.
Then if a user wants to drill down further and add inline editing, and that inline editing has a variant collection of inputs, selects, radio/checkbox assignments based upon the scope.object.array.item - well the alphabetical search fails miserably, as does the global search for numeric values, especially if certain fields are hidden by display management. -> I have any field with an id hidden by default, because I have no reason to render the id of inline editing elements which will convert into a select list drop down which renders from a completely different scope (for the sale of the select list) while still using the items scope as the value.
Obviously because the $index on search fails as pagination has created entirely new scope elements per row, per element - it always returns the incorrect record - forcing a
for(in this row loop to capture the id)scenario.
The point is to use the existing scope. Not duplicate the scope wasting resources.
I think that the pagination script should be checking the scope object, not deciding to create new scope elements and littering the scope with duplicate items.
There are a lot of issues with $scope.$watch and $scope.$watchCollection using your method.
I realize that you toss things here based upon your existing project, but to declare this to be a 'paginate almost anything' really means 'page only single array objects, but it's useless on any nested array object'
I used lint to go through your code to find out why - and that was when I noticed hey - most of these functions are entirely disconnected, and rather pointless as a 'universal solution' because the filters and directives - rather than being built as filters and directives - are all tossed into the service with the filters and directives referenced but not actually encapsulated within those filters and directives.
There is nothing wrong with someone else examining code - i never get my feelings hurt because someone else pointed out a flaw in my code so that I could (and do) correct it to make it better code.
And looking at the list of issues you ignore within your 'issues' section it is quite clear that you know this isn't the best you could have done.
michaelbromley
commented
8 years ago I'm sorry, but I genuinely don't understand most of what you wrote above. Perhaps that is a result of your 25+ years experience vs. my few. If you are so inclined and are able to reproduce a specific behaviour in a plunker demo, that would make it clear to me and also should be an actionable item for my growling list of open issues :wink:
"And looking at the list of issues you ignore within your 'issues' section it is quite clear that you know this isn't the best you could have done."
The reason the open issues list is longer than I'd like is a combination of a) lack of free time to spend triaging them b) waiting for information from the originator and b) some are genuinely very difficult and would require an inordinate amount of time to resolve. But also note that there are 219 closed vs 34 open at this writing.
If you choose not to open another specific issue regarding the faulty behaviour you outlined above, then I bid you farewell and good luck with your rewrite. :+1:
ghost
commented
8 years ago Lmfao at this thread. Is this real life?
jjravleuven
commented
8 years ago @Dillybob92 yup it is real. you know a lot of "coders" call themselves programmers, but let's face it they're not. they surf google, grab code written by other people, then attempt to integrate it into whatever project they're working on and then turn around and tell everyone they're a "programmer"
That's no different than putting fuel in your car at a gas station and telling everyone you're a mechanic, or putting WD40 in a squeaky door and telling people you are in renovations.
And the funny thing about that is that there is really a lot of code out there, which is propagated as "one size fits all" (like this particular script) yet is littered with errors, littered with warnings and so on.
Anyway, I rewrote it. It works for server side paging (the manner by which I'm using it) as well as my need for view type changes such as tables, panels, tiles, tables with nested object tables, and also works now with more complicated objects, such as nested javascript objects, and id/value key/value assignment pairs for inline editing (without using another google search based inline editor that is angular oriented - please see last paragraph).
Before buddy decided to be all dickwad condescending, I was going to share it with everyone, as a pull request, but then i thought fuck it. Why would I want to propagate and encourage people to call themselves programmers when all they do is surf the web and then jam other people's code calling it programming"
so yeah after looking at the list of errors which simply don't (and shouldn't) exist in my code apps... having zero errors, warnings in code is what we are SUPPOSE to be doing. But that's the difference between getting a pay cheque and building apps that companies buy from us... I'd rather build and sell it and move onto the next new idea then be 80 and I'm still working for someone else being a 1/2 assed faker who calls myself a "programmer" lol
But then again... I don't have a single html partial or segment and I only have 2 routes in all my angular applications - because the reality is - you don't need the retarded (not really programming) .when('build a new html page and then a new controller, and then a new service, and factory, and resource every time a client wants to add something) to the angular application, and my apps can be up and running and fully client facing in 72 hours - so yeah this is real life.
But enjoy taking other people's code slamming it into your app while making excuses to your clients as to why they can't do this, or that because you can't actually refactor the code you're ripping from other people lol
and btw - when I deal w/ coders? because I CAN code? I fire them if they choose to give me bullshit excuses for errors and warnings, instead of just writing GOOD code. I got into IT because of the 1/2 assed way those who like to call themselves programmers have the "this is good enough" approach to coding. Either do something well or get a job at McDonalds. period
michaelbromley
commented
8 years ago I'm gonna lock this thread now to protect the sanity of all involved. Let's get on with out lives.
So i have this thing about running code I want to use through linter's and this code is rife with tons of problems... and I'm curious why you didn't check it yourself... however, I have listed quite a few for you
And through an online linter which indicates much of the code is never called at all: