search

Glain / FFTPatcher

FFTPatcher suite project continuation. Original commit is .483 RC 2
GNU General Public License v3.0
31 stars 9 forks source link

Remove static in `FFTPatch`. #2

Closed Raijinili closed 4 years ago

Raijinili commented 7 years ago

It blocks code reuse. It is currently not possible to have two different versions of FFTPatch in memory at the same time, because no FFTPatch objects are created.

Glain commented 7 years ago

FFTPatch is a utility class containing static methods and variables that are helpful for patching. There's no need to instantiate it. (This class could probably have a better name.)

PatchedByteArray (and, by extension, AsmPatch) is probably the class you're looking for if you want a class that represents a single patch containing changes that could be made to an ISO, save state, etc.

Raijinili commented 7 years ago

No, it is a class that effectively represents a patch. Its static variables are effectively globalized instance variables of a globalized singleton object. Its static methods are acting on the patch "object". And there's no real reason for the static here.

I used IronPython to extract data from the patcher. To hold both US PSX and US PSP data in memory at the same time, I constructed a patch for one version, copied out its instance variables, and reconstructed it for the other version. It's a workaround for a misuse of static.

There's no real use for losing static now, but it's not The Right Thing and should eventually be fixed. The question is, "Does it make sense that only one patch exists at a time?" While it (currently) makes sense for only one patch to be represented at a time, the answer is no. Making it a real class would eventually allow for things like patch-diffing, or for the code to be reused in other projects.

I started work on removing static, but lost interest or something.

Glain commented 7 years ago

Oh, now that I look at it closer, you're right... FFTPatch is using those static variables to store a ton of loaded data, and is being used a lot like a singleton. What's confusing is that it also contains static utility methods, hence my first impression of it...

It does look like a bit of a mess that ideally should be changed, but as you alluded to, I'm not sure there's much point in changing it now unless we start working on a feature that requires it.

Raijinili commented 4 years ago

Fixed in 7dad67ed73e5386ca7086aa4bdf83acb5d10455c.