Identify and eliminate code duplication (100+ lines)BRL-CAD
Status: ClosedTime to complete: 72 hrs Mentors: SeanTags: C, programming, refactoring, reduction

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.

Uploaded Work
File name/URLFile sizeDate submitted
http://sourceforge.net/p/brlcad/code/59152/n/aDecember 24 2013 15:38 UTC
Comments
Johannes Schulteon December 23 2013 11:37 UTCTask Claimed

I would like to work on this task.

Johannes Schulteon December 23 2013 13:48 UTC

If I have a code duplication between libged and a specific primitive of librt, where should I put the function to resolve this. Is putting it into librt and then including this file into libged fine?

Sean on December 24 2013 05:11 UTCTask Assigned

This task has been assigned to Johannes Schulte. You have 72 hours to complete this task, good luck!

Johannes Schulteon December 24 2013 15:38 UTCReady for review

The work on this task is ready to be reviewed.

Johannes Schulteon December 24 2013 16:11 UTCFurther questions

I have some questions for the other code duplication tasks:


many of the work on poly-faces is duplicated for each primitive, e.g. converting plane eq to points or finding the centroid of a poly face. where should I put the code, so that it's reachable for every primitive.



In libged's analyze.c is much of the work from librt's primitives done a second time. While you could solve that easily by calling librt's functions through the OBJ functab, you have the problem, that currently analyze also displays the surface area of each face. thats something, we can't offer through the fixed function header for surface functions in librt. So whats more important, reducing the code a lot or keeping this functionality?

Sean on December 25 2013 18:53 UTCwhere

If there's duplication in librt and libged, then that usually means we need to create a new public function (obviously, heh), but the question of where depends on the data.  If it can be done without being specific to a primitive but is related to geometry objects, it probably belongs in librt (e.g., a function specific to a torus).  Code for converting simpler data like plane equations to points and such belong in libbn (basic numerics).  If it's polygonal and doesn't fit in libbn, then it probably belongs in a library we don't yet have separated out (libnmg).


Ideally we do not want to introduce new public API that is specific to a primitive (in librt or elsewhere), but creating public API that is just the underlying data (e.g., an array of vertices or plane equations or whatever) with libged and librt converting that data to whatever "private" container needed.  If the underlying data is really simple, it goes in libbn.  If it's not simple, it goes into librt.  Just depends, but it's best when the interface can be made completely agnotistic to any given primtive.


Case by case basis.  If it's unclear, just "design" the API first by making the header changes and documenting how the function should behave (that's a task in itself to get right if it's complicated).  That usually makes it a lot easier to discuss and consider.  You'd do this as an API design task instead of a code reduction task because we really want you to think a couple hours about how that API should be designed, not just make the quick and dirty code changes. ;)


 


 

Sean on December 25 2013 18:57 UTCTask Closed

Congratulations, this task has been completed successfully.

Sean on December 25 2013 18:58 UTClibbn

Looking at this patch, I'd say the refactoring really begs for a new libbn function.  If you want to tackle that design (something simple, clean), I'll create a task for it.  Ideally, new API needs to be usable in at least two places, ideally three.


 

Johannes Schulteon December 25 2013 20:19 UTC

Yep, this task sounds interesting, please create it. At the moment I would say, that part of the api-extension would be a definition of the polyface struct, a function to convert for an array of poly faces out of their plane eq's their point_t's and a function to calculate the centroid of one poly_face. Iirc this would be used in 4 places.

Sean on December 29 2013 04:53 UTCfollow-on

http://www.google-melange.com/gci/task/view/google/gci2013/5863528074313728