MEGA65 / mega65-tools

Tools and Utilities for the MEGA65 Retro Computers
GNU General Public License v3.0
30 stars 33 forks source link

Refactoring mega65_ftp #209

Open nobruinfo opened 1 week ago

nobruinfo commented 1 week ago

Is your feature request related to a problem? Please describe.

Upon realising #22 it was apparent the single source file building this file and slot transfer utility suffers from historical growth. It could be split into different areas of duty becoming a couple of source files. Another pain point is the extensive use of global variables to maintain info on what is currently accessed and where on the (virtual) storage for the MEGA65. I had to back those up into a set of shadow variables only to later restore those values where they globally belonged. A more object oriented approach in this standard C application should be possible. Along this step further unit testing should be established to allow frequent and regression free releases without adding new features while this is in the works.

Describe the solution you'd like

At the moment I had to copy-paste code and functions like the ones for showing the directory listing, counting its entries and deleting the same structures are cloned and only slightly altered code. I'd like to modularise these lines and encapsulate code into hierarchically well formed and easy to understand smaller units.

Describe alternatives you've considered

Leaving as is and further add stuff is possible. The file is getting bigger and bigger. The main culprit being functions no longer being collected in groups of duty makes it difficult to dive into. The good thing is that there is no hurry.

Additional context

This is no work issue ticket to directly rely pull requests upon. It is an initial discussion towards the whole team whether or not the goal of this loose list of things is a direction to head to. I'd be happy to read other ideas I did not think of. You can call it an epic for that matter.

gurcei commented 1 week ago

For me, I feel like maybe tackle refactoring in a piecemeal fashion, as you observe some aspect of the code that warrants refactoring. Let's say, in this case, you felt the pains of the global variables, so on this iteration, if you felt like addressing some of that (I'm guessing addressing all of it at once might be a chunkier piece of work), maybe it'd work that way.

It'd be some comfort to have some tests around in relation to the code being refactored, but hey, I accept we're all working with what free time we can spare on this labour of love, so I can understand if extra tests add too much of a drain on free time.

nobruinfo commented 1 week ago

@gurcei Two good points, thank you for your opinion. Of course first I need to further familiarise myself with the testing. In no way I would risk beginning sorting things up before having added eventually missing parts of the test for all of it.

I fully agree a refactoring can't be done in one huge single step. Such a pull request could not be handled. That said I still see the splitting all the functions of mega65_ftp into several files as the primary step, becoming a bunch of .h and .c in this step. Maybe:

Do you think we should ask other members of the team specifically? Who is still here being previously active in mega65-tools ?

gurcei commented 1 week ago

I think the most active devs on mega65_ftp over its history have been @gardners (the creator), @ki-bo and myself.

If you see a pathway to break out the code into multiple files based on purpose, sure that sounds good.

One little (but not insurmountable) hurdle in relation to this: presently, the program is predominantly in the file located at src/tools/mega65_ftp.c (alongside many other one-file tools at this path).

So if it breaks out into multiple files purely for mega65_ftp, putting them all within this path pollutes that path even further. I then wonder if it warrants putting them all within a new sub-folder at the path src/tools/mega65_ftp/. E.g., within here, there could be something like:

I'm guessing the Makefiles will need to be tweaked to suit such an arrangement, so some hurdles to get over, but do-able :)

nobruinfo commented 1 week ago

@gurcei Fear not, I would have put them in a subdirectory. There are already six tools like that. m65dbg is one of it. Another way would have been to prefix all the files with mega65_ftp_main.c or the like. Files like ethernet.c might vanish their prefixes as they could be used by different tools interchangeably. Many ideas, I never argue about such naming standards and will always adapt what I'm being told.

So let's wait if other want to chime in and have a word then. Meanwhile I'm adding some breaking tests into a local branch of mine to familiarise myself on gtest's behaviours and whims. 🤪