My favorites | Sign in
Project Home Downloads Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 1087: [Patch] Code cleanup
2 people starred this issue and may be notified of changes. Back to list
Status:  WontFix
Owner:  ----
Closed:  Nov 2011


Sign in to add a comment
 
Reported by Donu...@gmail.com, Jun 18, 2011
I really would like to understand what you are doing and how this whole thing works.
And here is the problem: Reading the code is not even close to easy.

What steps will reproduce the problem?
1. Open Eclipse
2. Download source code from repository
3. Look at the code

What is the expected output? What do you see instead?
Expected output (voice output from prospective Programmer) would be something like: "Nice" or "Cool" or even "Great"
Instead: "WTF???"

What version of the product are you using? On what operating system?
current trunk

Please provide any additional information below and PLEASE JOIN debug.log file !!

I am willing to do something about this. As a sign of my good will I add a little patch where I *refactored* the code a little. Of course I only changed the sources where I was sure I won't change mechanics.
refactoring.patch
7.3 KB   View   Download
Jun 18, 2011
Project Member #1 chocol...@cpan.org
Thanks for the patch. Code cleanup patches and documentation are always welcome.

Please note that functional changes are more likely to be applied if packaged as separate patches (i.e. so they can be tested and, if necessary reverted, separately). i.e. please don't mix them with uncontentious static cleanup that e.g. narrows down exceptions, removes unthrown exceptions, fixes Eclipse warnings &c.

Also, cosmetic changes such as:

> error in parsing unimportant metadata

- aren't code quality issues and come with a price. As it currently stands, PMS logs everything (i.e. all log levels), and adding "it's an error but it's harmless" messages such as this just results in users seeing "error" and reporting non-issues.
Jun 18, 2011
Project Member #2 chocol...@cpan.org
(No comment was entered for this change.)
Labels: -Type-Defect Type-Enhancement
Jun 18, 2011
#3 subjunk
I agree with chocolateboy's assessment; it would be better to keep cleanup and functional patches separate.
Jul 20, 2011
Project Member #4 chocol...@cpan.org
(No comment was entered for this change.)
Status: WontFix
Jul 20, 2011
Project Member #5 chocol...@cpan.org
Happy to reopen this if you get a chance to update the patch(es) as per the comments above.
Jul 23, 2011
#6 Donu...@gmail.com
I updated two of my patches. The first one is concerning the check for division by zero (see attachement divisionByZero.patch)

The second one is a refactoring of the method usn() in PMS.java (see attachement refactorUUID.pacht). This is just a proposal how I would do this. Imho this gives a better understanding of what happens there. I hope I understood the method correct and this is a true refactoring. 
divisionByZero.patch
629 bytes   View   Download
refactorUUID.patch
3.7 KB   View   Download
Jul 23, 2011
Project Member #7 chocol...@cpan.org
(No comment was entered for this change.)
Status: New
Jul 23, 2011
Project Member #8 chocol...@cpan.org
(No comment was entered for this change.)
Summary: [Patch] Code cleanup
Jul 26, 2011
#9 Donu...@gmail.com
I refactored another method. The changes are rather small, but I hope they will help anyone to understand this method more quickly. 

Feedback (positive or negative - both are welcome) on my refactorings would be very nice.
refactoring_getVideoBitrateConfig.patch
3.1 KB   View   Download
Aug 14, 2011
Project Member #10 Patrick....@gmail.com
Hmmm... I personally don't like the coding style of the "refactoring_getVideoBitrateConfig.patch". You seem to be aiming for less characters, where I'm more of a more characters guy. I like my variable names long and descriptive and code like "if (x > 7000) { x = 7000 }" instead of "x = Math.min(x, 7000)".

But hey, that's me. Each to his own style! :-)

I did like the addition of comments.
Aug 14, 2011
#11 subjunk
Yeah I agree with Patrick, I like code to be as easily readable as possible, especially in open source projects like this.
Aug 15, 2011
#12 Donu...@gmail.com
First of all thanks for feedback :-)

I was not specifically aiming for less characters. Fragments like "x = Math.min(x, 7000);" are just the way _I_ think the code becomes easier to understand. I have to admit that naming the variable "br" might not be the best choice but I wasn't creative enough and I really wanted to change the fact that the parameter was changed.

Please have a look at the other two patches too (and while you are at this anyway I have one more patch pending in the forum ;-)
I really would like to contribute to this project, but it's kind of difficult if I don't know "guidelines" or the like.
Nov 5, 2011
#13 subjunk
If you update the patch for the latest codebase we can have another look at it :)
Nov 6, 2011
#14 Donu...@gmail.com
I see what I can do :-)
Nov 6, 2011
Project Member #15 chocol...@cpan.org
As before, happy to reopen this if you get a chance to update the patch(es) as per the comments above.
Status: WontFix
Sign in to add a comment

Powered by Google Project Hosting