Rename Oddly_Separated_Classes to OddlySeparatedClasses

Bug #1343297 reported by SirVer
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Fix Released
Low
Unassigned

Bug Description

Legacy Widelands code still has classes/types with underscores in their name. They should be renamed. Also the filenames they are contained in should be unified (i.e. io/fileread should be io/file_read).

Related branches

Revision history for this message
SirVer (sirver) wrote :

Of course a codecheck rule should be added to make sure they never come back.

Changed in widelands:
status: New → Confirmed
SirVer (sirver)
tags: added: cleanups lowhangingfruit
Changed in widelands:
importance: Undecided → Low
Revision history for this message
GunChleoc (gunchleoc) wrote :

Won't that make filenames like "widelands_map_player_names_and_tribes_data_packet.cc" pretty much unreadable?

Changed in widelands:
assignee: nobody → GunChleoc (gunchleoc)
Revision history for this message
Hans Joachim Desserud (hjd) wrote :

That's a good point. I'm not really familiar with the data_packets, but would it be possible to give them some better, more descriptive names?

Another thing is that we have several files which outline their place/hierarchy in the file name. Take for instance src/editor/ui_menus/editor_main_menu_map_options.h. I'd say that the "editor_" and "main_menu" prefixes are unnecessary given the directory (and probably also context) the file exists in. There might be other places where this happens too, but the files related to the editor seem to do it a lot. This also goes for the various files which are prefixed "widelands_".

Revision history for this message
SirVer (sirver) wrote :

> Won't that make filenames like "widelands_map_player_names_and_tribes_data_packet.cc" pretty much unreadable?

wmpnatdp is significantly less readable to me. Also (WL)MOS is harder to understand than widelands_map_object_saver.

> I'm not really familiar with the data_packets, but would it be possible to give them some better, more descriptive names?

Sure. If you can come up with some you can simply replace the names. Filenames and class names should stay consistent though.

[reduntant parts in file names]
This applies also to game_io/*, map_io/* and for some other files too. Feel free to bzr mv those files, but as said, class names and file names should stay consistent.

Revision history for this message
GunChleoc (gunchleoc) wrote :

We also have to be careful about name conflicts - the same file might exist in various directories if we drop the prefixes. I guess the convention is that the filename always is a match for the class name?

What I find hard to navigate in the file system is stuff like this:

widelands_map_flag_data_packet
widelands_map_flagdata_data_packet

Can the data_packet suffix be simplified to something that makes sense, e.g. drop the "data" or the "packet" part?

The widelands_prefix can certainly go.

Revision history for this message
SirVer (sirver) wrote :

> the same file might exist in various directories if we drop the prefixes

should not be a problem. We have several constanst.h already now.

> I guess the convention is that the filename always is a match for the class name?

Yes, it is a convention. And it is not always easy to keep it. For example some files contain many classes (animation.cc for example).

widelands_map_flag_data_packet -> flag_packet
widelands_map_flagdata_data_packet -> flagdata_packet

I would not drop the packet, because that is what those classes are: packets in a file. The data is pretty redundant of course. not sure what the flagdata packet actually contains - maybe it can be renamed to something more meaningful.

Revision history for this message
GunChleoc (gunchleoc) wrote :

What about class names like EOT_Impl? Does the uppercase stuff need special treatment?

Revision history for this message
SirVer (sirver) wrote :

Generally speaking, abbreviations are bad. So this should probably be EndOfTextImplementation. Sometimes, abbreviations are so common, that you would never expand them. HTML Parser for example. In that case treading abbreviations as a normal Word is easiest to remember and and the most consistent: HtmlParser.

GunChleoc (gunchleoc)
Changed in widelands:
status: Confirmed → In Progress
milestone: none → build19-rc1
GunChleoc (gunchleoc)
Changed in widelands:
status: In Progress → Fix Committed
GunChleoc (gunchleoc)
Changed in widelands:
assignee: GunChleoc (gunchleoc) → nobody
GunChleoc (gunchleoc)
Changed in widelands:
status: Fix Committed → Fix Released
Revision history for this message
GunChleoc (gunchleoc) wrote :

Fixed in build19-rc1.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.