nds-bootstrap icon indicating copy to clipboard operation
nds-bootstrap copied to clipboard

Refactoring question - duplicated code

Open clach04 opened this issue 3 years ago • 1 comments

Your console:

NA not a bug report

Launch Method:

NA not a bug report

Game tested/used/applicable:

NA not a bug report

Expected Behavior:

Browsing through the code there appears to be some code duplication, which means more maintenance overhead, I wasn't sure if this was deliberate so thought I'd ask :)

For example, the new manual loading support has both a DSI and DS implementation of readManual():

  • https://github.com/DS-Homebrew/nds-bootstrap/blob/7979355fa40db3e59afcdb7842571d740d60ccc8/retail/cardenginei/arm7_dsiware/source/cardengine.c#L530
  • https://github.com/DS-Homebrew/nds-bootstrap/blob/7979355fa40db3e59afcdb7842571d740d60ccc8/retail/cardenginei/arm7/source/cardengine.c#L701

From a quick scan the only difference appears to related to drive/disk/FAT access:

driveInitialize();

versus

sdRead = (valueBits & b_dsiSD);

Any thoughts on having a shared code base, for some functions? Either complete module that's shared with #ifdefs or single function files that are included via the pre-processor, e.g.:

#include "../../......./shared/manual.c" 

E.g. something along the lines of:

void readManual(int line) {
#ifdef DSI_macro
    driveInitialize();
#else
    sdRead = (valueBits & b_dsiSD);
#endif
    char buffer[32];

    // Seek for desired line
    bool firstLoop = true;
    while(currentManualLine != line) {

This maybe a poor example, an alternative idea would be to add a driveInitialize() function to the non-DSI code and updates the global sdRead so there would be no need for ifdefs in the readManual() function.

Again, this maybe a deliberate design decision, hence the question :-)

Actual Behavior:

NA not a bug report

Steps to reproduce

NA not a bug report

nds-bootstrap.ini

Logs

Other notes

clach04 avatar Apr 23 '22 16:04 clach04

I don't think this is deliberate in the sense that there's a reason to have it like this other than convenience. I think the better option would be to declare driveInitialize for the non-DSi codebase, so that we wouldn't need a macro in global functions.

NightScript370 avatar Apr 24 '22 17:04 NightScript370