Improve code quality - fix security and performance issues, better error handling #613
Conversation
unclosed input buffer can be used as a vector of DoS attact on the application, cause memory leaks You can NOT realy on any JRE to automatically close streams and buffers!
- rename methods to follow java code guidelines - remove misleading comment - change system out printl to logger
some levels of loggins were too high for their purpose
code formatting changes to follow java code guidelines logger which doest not lose context of exception remove unnecessary and stupid comments comment out code which is never used
fix context for logger in opencellidactivity
this class should be 100% revwritten!!
better naming improved NPE checking memory usage optimisation
it was not called on seperate thread https://cwe.mitre.org/data/definitions/572.html
- better code formatting - logging exceptions...
better code organisation, less try/catch blocks
|
Guys, what do you do with time saved on:
|
also, removed tones of unused code in other classes
|
If you did quality and performance checks and do error-handling properly you would have not to do it now... There are still 2 memory leaks that I didn't bother fixing. |
agilob
changed the title
|
@agilob, thank you so much for your huge work here! I've been away for longer than I should have been, life has had a tight grip on me - but luckily I found a new job and I'm working my way back here so that I can continue to add my unicorn comments in the future as well. Enough chit-chat, MERGED! 👍 |
Improve code quality - fix security and performance issues, better error handling
|
And why are you wasting your time and ours, rewriting all the log Tags without even consulting us why we changed them in the first place? You seem to spend considerable time "fixing" things that is working... It would have been more appreciated if you can look at the issues already filed. |
|
And Sec, why do you just merge stuff without looking at the code or at least asking for a second opinion? |
|
This is really pissing everyone off. You've essentially removed every piece of code that is still work in progress, in addition to reformatting everything to your own taste without any thoughts or respect of why we have the formatting we do... |
Sorry, do you know what master branch is? It's dedicated for stable code that passed review process, test and is production ready, 3/4 of classes in this project should be removed and rewritten from scratch.
I'm not a developer of this project, I don't have contributor status, I don't even follow this project. I am a technical user of this project and I fixed what I found necessary to fix. Why do you invent your own logging paradigms? That are not efficient, are misleading (class named XX presents itself as YY!), not only loggers but own "standard" of comments that is it not even similar to javadoc or doxygen? This is just stupid and puts people off developing as it makes impossible to use any IDE with auto formatting. About formatting, why do you mix k&r, c++ and java coding standards? If you don't bother following globally known by beginners java coding style, give astyle a try (or try using an IDE like IDEA or Eclipse). It's easy and does formatting job for you. Why do you mix C style (lack of encapsulation, static global booleans in non-static class, static protected threads?!, static thread handlers (I didn't bother fixing it, I left if for you - DoS vulnerability and memory leak, find them), on inefficient wrapper for SharedPreferences? Do you know what lint or sonarqube is? It's finds elementary-level bugs in your code for you. Remember when due to lack of time I posted you 5 exceptions? 4 of them are fixed here, two are related to race conditions which happen on slower devices. You just closed the issue like an asshole. I recommend you using ACRA or anything else similar, you will be flooded by reports with same details I provided you there and no contact back to a user! It's awesome, makes you think before you commit untested and not analysed shit. You want a code review now? Did you make any before? Did you read that syntactically broken XML I found a few days ago, which is in repo for months (if you compile code with show warnings option , compiler will point you there - it's really easy!) Who did a code review of this e8e3769? I bet no one because no one who works with git or java would never let this commit be merge to master, or maybe you pushed it directly to master? Is it egoistic? Does it introduce vulnerabilities? Is it low quality code? Did you know not to push white-space changes and commented out code to git? It's just misleading, increases repository size, makes it harder to find who pushed bugs to master and who accepted it. If you think this PR should be reverted, do it, you have access to do it so and please don't contact me about this issue again, we don't have to waste each others time. |
As long as you don't do it. We already know that, and its all WIP. Deleting WIP code you think is dumb or useless is not how to do it.
That's exactly why you shouldn't remove or edit code you don't know what it is for, or what its status is.
This is not a standard hello-world project conforming to Googles out-of-their-ass guidelines and imperialistic IDE/UI/UX designs. There are specific reasons why we spent a lot of time making our own logtags. First of all for readability and clarity, second for automation and easy finding the code where there are bugs, if you've ever bothered to look at the logcat from command line, you know 2 things; 1) It's fucking impossible to know what app produced a specific log entry without having the PID on the live system. 2) The log TAGs have a size limit of 23 characters, and if you bothered counting, we exceed these quite often. With our tags, a simple
Thank you that explains you philosophy very well. Where you can actually do something right, you don't bother.
We asked you repeatedly to get back to us with more details, and there were silence, so yes we closed it.
I completely agree with you here. I'm actually totally fed up with this project exactly for this reason. I've been trying to have code review and getting people involved in the quality, but since we only have half a developer at any one time, that is not possible. Result is shit code hacked together and heedlessly "fixed" beyond any logic or scrutiny, which is exactly why I'm having this discussion with you. (And yes that code snippet was reviewed by @banjaxbanjo .)
It's not that black and white. Our biggest problem is that our code lacks comments and detailed functional diagram. The recent changes to the DB at a time when people and devs don't have time, is making things much worse. The DB migration is still not finished as the queries are not recognizing linked tables.
I think it should be reverted, not because you didn't put a lot of work into this, but because you lack insight to what the stuff you changed and reformatted, was/is meant for. Instead of making many small PRs fixing specific details, you decided to make one huge PR, that is trying to fix too many things concurrently while unknowingly breaking a bunch of stuff which will be impossible to find since you decided to remove comments and code, or anything else that can be useful in a code review. Remember: Less is more! = One fix, one PR. |
Great, and now I feel bad again. I've been away for like ages, had a very hard time coming back and now people like @agilob who essentially put so much love into our app and work on the repo get blamed again for doing anything. Not cool! Let us NOT continue this discussion here, please. I think we all got the point now: Smaller pull requests are better and I will from now on ALWAYS make sure to request some sort of confirmation into each PR before merging it. Additionally, I turned on I would like to keep the work done here though. We really ought to make this repo a place where standards get settled and developers feel comfortable. Would you PLEASE do me the favor and give the latest Buildozer build a test? Did anything change to the worse? Works smooth as butter over here! @He3556 what are your test results - anything to add? Thanks for staying calm, especially @E3V3A. |
|
I get errors about "Missing Translation" since we are working on the translations. Sorry i am good in ignoring errors as long as the App compiles and runs on the phone. Everybody should know about that errors anyway... |
This PR contains commits that improve code quality, fix security, performance issues and fixes really bad/unpredicted code behaviour. Functionality should not change at all. See commits for more.