Skip to content
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

Split up "god" classes, big source files #49

Closed
cxong opened this issue Feb 5, 2013 · 10 comments
Closed

Split up "god" classes, big source files #49

cxong opened this issue Feb 5, 2013 · 10 comments
Labels
Milestone

Comments

@cxong
Copy link
Owner

cxong commented Feb 5, 2013

There are a few source files that are >10kb in size, which is highly suspect. No module should need to be this big. They may be big because they contain large, hand-coded data structures (which can be separated from logic), or they have multiple functionality.

Currently the following source files exceed 10kb in size:

  • actors.c (26kb)
  • ai.c (14kb)
  • cdogs.c (27kb)
  • cdogsed.c (39kb)
  • charsed.c (17kb)
  • files.c (20kb)
  • game.c (15kb)
  • mainmenu.c (26kb)
  • map.c (29kb)
  • mission.c (19kb)
  • objs.c (27kb)
  • pics.c (21kb)
  • prep.c (24kb)

This is almost half of all the source files. Given that CDogs is a fairly big game for the number of source files, this is somewhat expected.

This will be a big ongoing issue. Keep updating this issue with findings such as:

  • why a certain source file is so big, i.e. hardcoded structures, or multiple functionality
  • any refactors done
  • common functionality found
@cxong
Copy link
Owner Author

cxong commented Feb 5, 2013

mainmenu.c has multiple functionality

  • menu system (drawing menu, handling menu input)
  • menus themselves
  • campaign/dogfight scanning, displaying in a menu
  • drawing other non-menu-item text

@cxong
Copy link
Owner Author

cxong commented Feb 22, 2013

To help refactoring, check out some call-graph visualisation tools: http://stackoverflow.com/a/517797/2038264

@cxong
Copy link
Owner Author

cxong commented Jun 3, 2013

There's lots of separate game loops implemented which follow the same pattern:

  • Blit background
  • Poll/update input devices
  • Handle input
  • Draw any other content (menus)
  • Sleep

And they are interspersed throughout all the game screen functions. The game loops should be refactored into a common loop with game screens. So far the game loops are in:

  • cdogs.c
  • cdogsed.c
  • chared.c
  • game.c
  • prep.c

which all happen to be large files.

@cxong
Copy link
Owner Author

cxong commented Jun 5, 2013

A better metric would be to use LOC from the ohloh analysis. Currently the top offenders are:

  • cdogsed.c 1325
  • map.c 1032
  • actors.c 955
  • cdogs.c 861
  • menu.c 824
  • prep.c 806
  • pics.c 782
  • charsed.c 583
  • files.c 529
  • game.c 511
  • ai.c 465

@cxong
Copy link
Owner Author

cxong commented Aug 7, 2013

Map is a big source file that actually contains different classes, which are used by things other than the map:

  • TileItem: something that stands on a map tile, such as objects, characters, and has dimensions. Actor is sort of a sub type, and contains a TileItem reference.
  • Tile, apart from the obvious contains a linked list of TileItems. Drawing uses this hierarchy
  • Buffer: for drawing, a "buffer" of tiles is copied from the map corresponding to the current camera view
  • Map

There are also other concepts here that whilst closely related to map, may be higher level than it:

  • Moving TileItems between tiles
  • Collision between TileItems
  • Map creation
  • Getting the explored percentage - a concept that relates to both the map and player "teams", although it's ok to be in the map for now

@cxong
Copy link
Owner Author

cxong commented Nov 25, 2013

Updates:

  • cdogsed.c: 1626 (+301): added a bunch of functionality (e.g. tooltips) without refactoring
  • map.c: 1094 (+62)
  • actors.c: 923 (-32)
  • cdogs.c: 1065 (+204): the 4 player handling routines have increased file size
  • menu.c: 1060 (+236): new menu types have been added, but the existing ones have not been refactored to take advantage of them; could drastically reduce file size by removing the multitude of menu types and moving into mainmenu.c
  • prep.c: 291 (-515): split into player_select_menus.c and weapon_menu.c
  • pics.c: 747 (-35)
  • charsed.c: 643 (+60)
  • files.c: 632 (+103): old/new format conversion functions added
  • game.c: 551 (-)
  • ai.c: 532 (+67): added meaningful helper functions but increased file size

New top offenders:

  • mission.c: 763: mostly due to the quick play feature; theoretically could be split but being something deeply tied to the mission/map types, is going to be problematic
  • objs.c: 1156: probably didn't show up in previous list due to omission; was always a big file
  • mainmenu.c: 533: more options means bigger menus; could be split into its constituent submenus
  • player_select_menus.c: 520: split from prep.c but still very large itself; contains the player template system which could be split

@cxong
Copy link
Owner Author

cxong commented Dec 9, 2013

ai.c has been split up; due to the addition of coop AI, some common utilities (such as getting closest enemy) have been moved to ai_utils.c, and coop only AI is in ai_coop.c. ai.c would eventually only contain code for the "classic" AI.

ai.c: 476 (-56)
ai_coop.c: 90
ai_utils.c: 286

cxong added a commit that referenced this issue Dec 15, 2013
Refactor menu to remove campaign type (#49)
@cxong
Copy link
Owner Author

cxong commented Jan 14, 2014

Due to recent editor refactors, cdogsed.c has shrunk but it created a new monster file - editor_ui.c, which creates all the UI elements.

ai.c: 480 (-52): many utility functions migrated to ai_utils.c
cdogsed.c: 962 (-664)
charsed.c: 507 (-136)
files.c: 480 (-152): removed old map format saving routines
game.c: 720 (+169): added frame skipping and secret weapons
player_select_menus.c: 360 (-160): player template split into player_template.c

New top offenders (500 LOC or greater):
editor_ui.c: 1693: verbose UI specifications; could be split and refactored but is still under rapid development
map_new.c: 561: only contains loading and saving of new JSON map format, may be able to make smaller if character loading/saving were refactored out

Top 10:
editor_ui.c: 1693
objs.c: 1233
map.c: 1199
cdogs.c: 1130
menu.c: 1108
actors.c: 1012
cdogsed.c: 962
mission.c: 796
pics.c: 747
game.c: 720

@cxong
Copy link
Owner Author

cxong commented Feb 18, 2014

Static map editing has been added, so despite some code being split from editor_ui.c to editor_ui_static.c, the former has still grown to over 2k lines! UI specifications are verbose by nature so I'm reluctant to refactor that further. Maybe more splits are in order.

editor_brush.c: 609: code for every tool; splitting it up will also clear up the multitude of large switches
editor_ui.c: 2034 (+341)
map.c: 1092 (-107): some loading has been split out into map_classic.c
map_new.c: 900 (+339): more features means more code
mission.c: 583 (-213): quickplay-related code has been split into quick_play.c, one of the rare wins this time around

New top offenders (500 LOC or greater):
ui_object.c: 511: a few extra features for UI objects has pushed this into 500+ LOC territory
blit.c: 589: a new blit highlight feature. There's a lot of duplication in the various blit routines so once the old pic formats are removed this can be shrunk by quite a bit.
draw.c: 517: highlights and editor-specific draw routines has enlarged this file. The character drawing routines can be refactored though, as it is almost a direct duplicate.
hud.c: 531: score updates were added, adding quite a bit chunk of code. Unfortunately the only possible splitting method is to split into a large collection of small source files, because that's what the HUD is: a large collection of small routines for each HUD element.

Top 10:
editor_ui.c: 2034
objs.c: 1273
menu.c: 1110
cdogs.c: 1153
map.c: 1092
actors.c: 1048
cdogsed.c: 956
map_new.c: 900
pics.c: 747
game.c: 723

cxong added a commit that referenced this issue Mar 16, 2014
Fix assertion with score 0 updates
cxong added a commit that referenced this issue Mar 16, 2014
@cxong
Copy link
Owner Author

cxong commented Jun 26, 2014

A large number of refactors and polishing this time around, with little effect on LOC counts.

editor_ui.c 2150 (+116) some editor enhancements and a huge file gets a bit bigger still. shikata ga nai
hud.c 813 (+282) the addition of score updates, compasses etc has made this into a top-10 LOC file. A split is in order.
objs.c 469 (-804) mostly split into bullet_class.c, which is on track to shrinking too 👍

New top offenders:

bullet_class.c 792 The big development this time round is the split of the previously-huge objs.c into this. Bullet class encapsulates bullets (everything that weapons shoot) and objs.c gets the scraps, which includes the non-moving TObject. There is hope for this getting smaller again, as the recent weapons refactor seeks to move all the bullet definitions into an external data file.
map_build.c 610 A great enhancement (changing tiles in-line instead of reloading the whole map) unfortunately bloats this file into a new top offender. Unsure where to proceed with this as map construction is an inherently complex task. Perhaps the placement of different tiles (walls, doors, rooms) can have their own "strategy" files?

Top 10:
editor_ui.c 2150
cdogs.c 1198
menu.c 1076
actors.c 1062
map.c 1027
cdogsed.c 1014
map_new.c 930
hud.c 813
bullet_class.c 792
pics.c 750

cxong added a commit that referenced this issue Jul 15, 2014
@cxong cxong closed this as completed May 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant