BRL-CAD is huge. With any large body of code, one inevitably ends up with a mix of good and bad coding practices. On the whole, BRL-CAD is actually better than most but we are constantly working on improving the code. This includes eliminating duplication.
References:
- http://en.wikipedia.org/wiki/Don't_repeat_yourself
- http://brlcad.org/wiki/Code_Cleanup
- http://brlcad.org/wiki/Compiling
- http://brlcad.org/wiki/SVN
This task involves reducing BRL-CAD's source code by 100 or more lines of code by refactoring and eliminating duplicate code. You can use whatever method you like to identify duplication, but beware that there are more than 1 million lines of code in BRL-CAD, so you're not likely going to find this duplication just by browsing.
We suggest using a code duplication detection tool like Simian. See our Code_Cleanup page for details.
Download our latest Subversion trunk sources and make sure you can compile cleanly first. Then you can run Simian or do whatever you need to find sources of code duplication. Make your edits, then make sure the code still compiles (run "make", "make test", "make regress" and "make benchmark" to test your changes). Finally, create and submit a patch file of your changes (see the references, svn will create the patch file for you).
Feel free to join the brlcad-devel mailing list or IRC channel to discuss your changes beforehand.
| File name/URL | File size | Date submitted | |
|---|---|---|---|
| duplicate_removal_2.diff | 49.8 KB | December 30 2013 12:32 UTC | |
| duplicate_removal_2.diff | 50.2 KB | December 30 2013 14:54 UTC | |
| duplicate_removal_2.diff | 50.1 KB | December 30 2013 14:59 UTC |
I would like to work on this task.
This task has been assigned to Andromeda Galaxy. You have 72 hours to complete this task, good luck!
I believe that several of the files that have both command.c and wdb_command.c implementations can be consolidated by turning the command.c function into a reference to the duplicated code in wdb_command.c; I would like to move the wdb_command.c file into command.c and add the unduplicated code in these cases (i.e. only one function is added); would this work?
FYI, we'll create as many of these as are completed, so feel free to work on more in advance while awaiting review. I just added a handful more.
You'll find that for many of the functions, it's not as simple as calling the wdb_ code but the bigger issue is that we eventually want the wdb_ ones to go away. That wdb_ code relies heavily on Tcl functions and we'd like the implementation to not use that approach (i.e., do like the other non-wdb code does). Eliminating the non-Tcl versions gets us farther from that goal, not closer.
It didn't matter for that Fg one you worked on, but it does matter for some of the others.
Thank you for that information. I think that I have found a way in track.c that does eliminate the implementations inside the wdb_* file for a bunch of functions that are almost the same in both files, and then implements the one function unique to wdb_track.c in terms of the functions in wdb.c, but it is taking a long time and will reduce duplication by several hundred lines. I do not want to submit an incomplete version as the code is somewhat incomprehensible; would it be possible to break this task into multiple tasks after I have completed the work?
The work on this task is ready to be reviewed.
We can certainly break up tasks into multiple when they take more than a few hours, but know that we already will be taking complexity in consideration when evaluating our grand prize candidates. We go over every task and assign them a rank based on complexity, quality, utility, and a couple other factors. If you are concerned about not making the top five, then by all means talk to us and let us know how much time a task took so we can consider breaking up the work.
For this patch, this looks pretty good. The only problem I see is that the old function you renamed to ged_track_g shouldn't be named with the ged_ prefix now that it's no longer public API. You can just call it track_g or _ged_track_g and put it's declaration into ged_private.h.
So just how long do you think this would take you if you had to do it again? How long did it take you this first time?
One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.
The work on this task is ready to be reviewed.
Sorry, I uploaded a partial diff, the proper one will be here in a minute.
I renamed ged_track_g to _ged_track, because the _g suffix is somewhat obscure; I think that your naming conventions let the leading _ denote a private helper version of the function.
Right now I am not too worried about getting into the top 5, so I don't mind leaving this as a single task.
Congratulations, this task has been completed successfully.