-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactoring mega65_ftp #209
Comments
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. |
@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
Do you think we should ask other members of the team specifically? Who is still here being previously active in |
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 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
I'm guessing the Makefiles will need to be tweaked to suit such an arrangement, so some hurdles to get over, but do-able :) |
@gurcei Fear not, I would have put them in a subdirectory. There are already six tools like that. 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. 🤪 |
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.
The text was updated successfully, but these errors were encountered: