Opened 6 years ago

Last modified 4 years ago

#3438 assigned enhancement

Split Autostart function into smaller pieces

Reported by: elexis Owned by: Anil Eapen
Priority: Nice to Have Milestone: Work In Progress
Component: Core engine Keywords:
Cc: Patch: Phab:D1554

Description (last modified by Arch Stanton)

The autostart() function in GameSetup.cpp has 282 lines of code. To improve the readability of that function, we should move the contents of the following if-statements into separate functions:

if (mapDirectory == L"random")
if (mapDirectory == L"scenarios" || mapDirectory == L"skirmishes")
if (args.Has("autostart-ai"))
if (args.Has("autostart-aidiff"))
if (args.Has("autostart-civ"))
if (args.Has("autostart-playername"))
if (args.Has("autostart-host"))
if (args.Has("autostart-client"))

Maybe moving this one from Init() would also make sense:

if (!args.Has("mod") && (flags & INIT_MODS) == INIT_MODS)

To allow easy reviewing, we could make one patch / commit per move.

Change History (11)

comment:1 by elexis, 6 years ago

If we are touching that code already, we should attempt to move the related functions including AutostartVisualReplay() to a new file Autostart.cpp, since Gamesetup.cpp already has more than 1500 lines.

comment:2 by elexis, 6 years ago

Keywords: simple added

comment:3 by Arch Stanton, 4 years ago

Description: modified (diff)
Owner: set to Arch Stanton
Status: newassigned

in reply to:  1 comment:4 by Arch Stanton, 4 years ago

Replying to elexis:

If we are touching that code already, we should attempt to move the related functions including AutostartVisualReplay() to a new file Autostart.cpp, since Gamesetup.cpp already has more than 1500 lines.

Before breaking off those if statements into functions, are you sure you don't want the entire Autostart() function moved to Autostart.cpp as a first step?

comment:5 by elexis, 4 years ago

You can try that in a branch and link it if it succeeds and shows an advantage over the current code.

(This ticket should only be a simple cleanup - in the long run / ideally we would have some kind of mechanism shared by gamesetup.js, Gamesetup.cpp and Atlas, so that we don't have to keep all 3 in sync when adding or chaning gamesettings.)

comment:6 by Arch Stanton, 4 years ago

@elexis, thanks for the response.

I got sidetracked and not sure when I'll be getting to this ticket. I'll unassign myself for now.

I hope I didn't cause any inconvenience in my delayed reply.

comment:7 by Arch Stanton, 4 years ago

Owner: Arch Stanton removed
Status: assignednew

comment:8 by elexis, 4 years ago

No inconveniences :-)

comment:9 by Anil Eapen, 4 years ago

Owner: set to Anil Eapen
Status: newassigned

comment:10 by Anil Eapen, 4 years ago

Please find for review 1st change at Phab:D1554

Started with

if (mapDirectory == L"random")

as separate function LoadRandomMap()

Last edited 4 years ago by Anil Eapen (previous) (diff)

comment:11 by elexis, 4 years ago

Keywords: simple removed
Milestone: BacklogWork In Progress
Patch: Phab:D1554
Note: See TracTickets for help on using tickets.