Fenris_Wolf Posted January 22, 2018 Share Posted January 22, 2018 Would somebody PLEASE refactor this function into multiple smaller functions? Apologies if already been done in IWBMS/vehicles branch, in 38.30 its 669 lines long. The massive length makes it impossible to override small parts of it via mods without worrying it will break on future updates or across all branches. My specific issue (which makes a perfect example) comes 423 lines into the function...where the weapon upgrades context menus come into play. I'd like to override this area, but can't without doing the whole thing. This part here is actually the source of my grief (434 lines in): part:getMountOn():contains(isWeapon:getDisplayName()) This doesn't allow individual firearms to have the display name changed without breaking upgrades. A simple tweak that would fix that, by fetching the display name from the original script item: part:getMountOn():contains(isWeapon:getScriptItem():getDisplayName()) But thats just my issue, but the truth is any 669 line function is overly excessive, and problematic for maintainability, error checking and bug hunting. I realize you guys are currently busy refactoring and optimizing more critical parts of the code base, I'd be more then happy to submit a refactored version if it allows you to continue the more important work uninterrupted. Link to comment Share on other sites More sharing options...
Connall Posted January 22, 2018 Share Posted January 22, 2018 Might want to cross post to here: https://theindiestone.com/forums/index.php?/topic/23315-the-modders-wishlist/ Less likely to be buried in there. However, if changing display names breaks a weapon upgrade, then that seems to be more of a bug than intended behavior? Unless I'm missing something. Link to comment Share on other sites More sharing options...
Fenris_Wolf Posted January 22, 2018 Author Share Posted January 22, 2018 Well I wouldn't technically classify it as a bug, unless it was intended that the display names could be changed and not break the upgrade context menu. Changing display names of individual firearms isn't exactly normal and unless this behavior was previously anticipated, then the code works as expected. My whole goal was to have it so some RP servers that manually spawn ORGM's firearm could edit stats/display names for RP puproses. But really my point wasn't about the display names/upgrade issue itself, but the length of the function and the impossibility of overwriting specific areas of it without having a mod break on different PZ branches and updates, since its highly likely this function would change overtime as new context menus are added in. Good idea on the crosspost. Link to comment Share on other sites More sharing options...
EnigmaGrey Posted January 22, 2018 Share Posted January 22, 2018 I'm sure there's larger functions lurking in 3000 line files. Pretty sure you can edit item names as an admin by right clicking and going into Edit. Don't know if that'll break upgrades. Link to comment Share on other sites More sharing options...
Fenris_Wolf Posted January 22, 2018 Author Share Posted January 22, 2018 Just now, EnigmaGrey said: Pretty sure you can edit item names as an admin by right clicking and going into Edit. Don't know if that'll break upgrades. In truth I haven't tested, but the code in the initial post above says it will break. :getMountOn() returns the gun display names the upgrade can be mounted on, and it checks if the item's current display name is in there, instead of the original display name. I'm willing to bet theres larger functions in the files too, I have yet to read all the base lua files. IMO large functions can be hazardous for development teams, especially true for functions that have a high probability for changing over time. Sometimes large functions simply cannot be avoided, in this specific case however a refactor would probably be in the best interest of both the team and modders. As stated I'd even be willing to donate my own time to do the grunt work it really wouldn't take me long, documenting the changes would take longer then the actual refactor. But since I'm a GOG user I only have access to the 38.30 branch and not the others, if the function has been changed then anything I do would already be partially outdated. Link to comment Share on other sites More sharing options...
Fenris_Wolf Posted January 22, 2018 Author Share Posted January 22, 2018 After a quick regex check (to satisfy my self-curiosity), there are 5977 functions in 504 files Counting only functions over 300 lines (not counting blank lines or comments), the largest functions in order are: MainOptions:create (1124 lines) ISWorldObjectContextMenu.createMenu (941 lines) MainScreen:instantiate (841 lines) ISInventoryPaneContextMenu.createMenu (669 lines) ISBlacksmithMenu.doBuildMenu (466 lines) ISInventoryPage:refreshBackpacks (386 lines) ServerList:create (378 lines) ISInventoryPane:renderdetails (332 lines) EDebug.DemoTime (327 lines) So ISInventoryPaneContextMenu.createMenu clearly isn't the largest, its the 4th. But with this info the same arguments for refactoring could be applied to ISWorldObjectContextMenu.createMenu as well (being almost 300 lines larger), probably even more so. Link to comment Share on other sites More sharing options...
Recommended Posts
Create an account or sign in to comment
You need to be a member in order to leave a comment
Create an account
Sign up for a new account in our community. It's easy!
Register a new accountSign in
Already have an account? Sign in here.
Sign In Now