search

sketchpunk / opencomicreader

Comic/Manga viewer for Android devices.
95 stars 37 forks source link

Cleanup codebase #40

Closed maximeh closed 10 years ago

maximeh commented 10 years ago

Hi

I know this pull request is huge, but it's only refactoring and cleaning a little bit the code, reformatting it and such. I have others pull requests that awaits the approval of this one, and I have much more stuff that I want to do.

Thanks a lot for this great app !

sketchpunk commented 10 years ago

Sorry, but I can't accept this pull request. A lot of what you took out or changed is my personal programming style. Looks like you tried to change the codebase to mimic a different coding style many people tend to follow. I find the way I write code and add comment borders helps in sectioning code and for me personally makes it more readable.

Again, I'm sorry that I have to decline it because I see that you spent quite a bit of time combing through all the code. I do appreciate the effort, but if you are doing such a massive change to any opensource project you should probably talk to the developer and ask why things are the way they are. This way you don't waste your time in the long run.

Sorry.

maximeh commented 10 years ago

Hi,

That's no big deal, I have a lot of tools to do that, it did not took long. And the other pull request I have have been made on your existing coding style, so I know I can push them know :).

Sorry for not contacting you before doing this, I should have done it, you're right.

I let you close it then ?

maximeh commented 10 years ago

Oops, seems like I closed it.

By the way, what about the Goto patches ? It seemed weird to me to read 'Goto', instead of 'Go to' Also the hardcoded title is fairly trivial. I can resend a PR with only theses two if you would accept them ?

I'll send the other later today.

sketchpunk commented 10 years ago

Feel free to send the one fixing the goto. I have a habit of writing it like that because I programmed in classic basic along time ago where Goto was a command :) The one about changing hardcoded titles into variables don't bother. Personally, If the text isn't going to be dynamic, no point of making a variable for it, keep it static.

Now this isn't really permanent and at some point will change though, Many of these titles will become variables when someone decides that want to add language support, at that point I'd work with whoever to figure out the best way to make it work. One person did try to add german to the app but they did it while I was redoing the library screen so all their work couldn't be merged in because I created a new activity and rebuild the screen from scratch, so the file they modified was going to be removed from the codebase. Think I did get an email recently if the app supports the Indian language, so I see that there is a slow demand for language support for the app.