search

moode-player / moode

moOde sources and configs
GNU General Public License v3.0
1.02k stars 167 forks source link

Code refactoring #467

Closed jeroenwalter closed 2 years ago

jeroenwalter commented 2 years ago

Hi I'm trying to understand the code, so I can add some new features. However, the code is difficult to navigate, lots of very large files, lots of code duplication and other code quality issues. While implementing my new feature, I have to restrain myself to not touch the current code, otherwise I don't expect a pull request to be accepted as it will then have too many changes not related to my new feature. Would it be acceptable if I try to make some refactoring steps and make pull requests from them separate from my new feature? I'm thinking about splitting some very large files into smaller files, for instance moode.php, playerlib.php and other large files. I don't know when I would have a first pull request though, as the lack of unit tests makes it harder to ensure everything keeps working. Which brings me to another issue, i.e. the lack of unit tests.... I might make a stab at adding some unit tests to my code in javascript and php, and while doing so, maybe also find out how to unit test the current code. My hope is that by doing these refactoring and unit testing, that not only will I gain more experience with the code, but also make it so code changes can be done more easily and more confidently, while also allowing other developers to more easily understand the code and contribute to it (I don't know if that's a huge problem now or not).

What is your take on this?

moodeaudio commented 2 years ago

The codebase certainly needs to be refactored, restructured and cleaned up but thats a truly huge undertaking and would require an intimate understanding of moOde as a whole including knowledge of the code, all the hacks, workarounds, corner cases and "reasons" why some things were done a specific way.

Your best bet is to try and make your feature within the limitations of the existing code. So far it's not been a major problem with dev's. Some very complex features and options have been added over the past years.

We have done some targeted code cleanup including splitting function libraries, using class libs for implementing certain features for example CamillaDSP, etc. You are welcome to propose specific code cleanup/restructuring actions but I'd prefer to see a list of what they are so they could be discussed.

As far as Unit Tests goes we don't have the project bandwidth (time) to go down that route. Our releases are integration tested and are usually of high quality. There are always some bugs which our users find pretty quickly but generally they report that the software is stable and reliable.

-Tim

jeroenwalter commented 2 years ago

Alright, then I will continue with what I'm doing now, add my new code while minimizing the changes to the current code. And maybe, once I have that running, I can find time to dig deeper into the current code and see if I can, after checking with you, refactor some things.

jeroenwalter commented 2 years ago

I've been developing my Collections feature now for some days and unfortunately it's slow going :( This is mainly because of the state of the existing code. Most of the time I spent was frustrating over the combination of jquery and php and the static tightly bound relationships between the php code and the html/javascript code.

I will finish a first working version of the Collection feature, but after that, I don't really feel like continuing development until some refactoring is done.

The Collections feature, as it is now, is sufficient for me, but I'd like to add some more small (or maybe large) improvements along the way.

So, if time permitting, I will start refactoring some code, starting with trying to split some large files into smaller ones, extract some classes perhaps. Also, I'd like a better client-side library like vue.js, instead of the current jquery-php interface. In my opinion the php code should mostly (maybe only?) present a REST API to the client application. Let the client code parse and generate html code, instead of php. I'd much rather prefer C# Blazor over php, but I think that's a bridge too far for now, I guess....? :)

It'll be a lot of work and I will only do that, if I know that you stand behind it and my efforts aren't in vain. Maybe start small, like extracting the playerSession function into a class and own file, then create a PR, so you can take a look at it.

moodeaudio commented 2 years ago

I'm not sure where you are going with this topic. I mentioned in my previous post that "Your best bet is to try and make your feature within the limitations of the existing code". Mixing in a new feature with code refactoring, new frameworks and new languages is a no-go.

jeroenwalter commented 2 years ago

It's not my intention to refactor along with the new feature, nor do I intent to introduce a new framework or language. I want my feature to be a small as possible with regards to changes to the current code base. I'm just stating, that I'm not eager to continue developing in the current code base, but that I'm willing to do some refactor work apart from the new feature. Maybe I should just first finish my feature and create a PR, so you can check it and then decide if I should continue or give me some pointers.

moodeaudio commented 2 years ago

In our Forum I mentioned that the feature idea has been added to the moOde 8 series TODO (Q1 2022). https://moodeaudio.org/forum/showthread.php?tid=4606&pid=38619#pid38619

I think it can be implemented within the existing Library search/filtering code as described below. It does not involve any PHP template screens.

jeroenwalter commented 2 years ago

I've read that, thanks for those suggestions. What I have now, however, works slightly different. My collections are a view on the database, using several logic-OR searches. When activating a collection, a different flatlist in a separate folder, containing the result of multiple searches, is generated and used. As this collection flatlist replaces the default flatlist (with the entire library), you can search in moode audio in the collection flatlist. Instead of a sql table, I use a json file for each collection. Mainly because I didn't want to add things in the database, without knowing the code base well enough and this was for me the fastest option, seeing as I already create new flatlist files on disk. I'm open for improvements here.

Check Collection configuration screenshot for what I just pushed. If you want, we can discuss what I created. I still have some minor issues with select/dropdown controls and their bootstrap counterpart for instance. I suggest we do that not in this thread, but in my repository's issue section. My branch for the collection feature: https://github.com/jeroenwalter/moode/tree/collections

moodeaudio commented 2 years ago

Regarding the feature please refer to Forum post https://moodeaudio.org/forum/showthread.php?tid=4606&pid=38683#pid38683

This issue has gotten off-topic and I've already suggested in earlier posts that while I agree that refactoring the codebase is highly desirable, doing it in a piecemeal fashion is a no-go. Also, migrating from PHP/JQuery to some other frameworks would involve a complete rewrite and thus is also a no-go.

moodeaudio commented 2 years ago

@jeroenwalter there may be opportunity for a Refactoring sub-project in Q1 2022 that involves creating a set of standards and a master plan to enable multiple devs to independently contribute to refactoring the codebase. If you are interested in this let me know and I'll add you to the Test Team sub-Forum.

jeroenwalter commented 2 years ago

@moodeaudio Good to hear that there might be something planned, I'm interested, so you may add me to the sub-forum. In the meantime, I'll take a stab at modifying my feature so it implements your proposed View feature you mentioned, as my code basically does the same thing, but via json files instead of sql for its configuration.