I would like to work on this task.

This task has been assigned to Marc Tannous. You have 100 hours to complete this task, good luck!

Hello,

I set up the tests for Cone's functions from Cone.cpp, now all I have to do is add them to my "main" function, where I run all of them and add the Cone to the database if it passes, just like in Sphere or Halfspace.

However, I encountered two issues :

1. There are 5 declarations of a Cone. Different parametres, however the Clone function is the same for all of them. Does this mean I run 5 tests on Clone, with all possible options? Also, the Set function has 5 declarations, for all 5 types. I only made 1 test for Clone and 1 for Set, so you can see if my logic is failing or not, but I wanted to know before coding all of that, maybe there's an alternative.

2. Why is height defined as a Vector3D? Also, why is the SemiPrincipalAxis one time declared as a size_t and then as a vector3D in the Set function?

Attached is my code so far, I need to fill in the main with all the tests just like I did in Sphere, and also add about 4+4 other functions for all the options of a Cone, if there is no alternative.

Regards,

Marc

The work on this task is ready to be reviewed.

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.

1. Those are five different constructors, so yes you should end up testing each of those constructor methods (either directly or indirectly).  If the constructors call the 5 different Set functions, then you can get away with just testing the constructors or the Set functions, but the point is to make sure they are all exercised.  This is a good one to discuss with andrei or daniel if you can get ahold of them to verify, but the intent of all unit testing is to test everything at least once.

2. A vector defines orientation and length, so while height could be described by a simple length, there'd still need to be a vector to describe the orientation.  So it just uses a vector which can encode both.  SemiPrincipalAxis is not declared as a size_t.  There's a SemiPrincipalAxis() function that takes a size_t, and this is an integer index for a particular axis that was recorded with SetSemiPrincipalAxis().  See the Cone.h header for more details.

The work on this task is ready to be reviewed.

Because it tests pretty much all possible Cone constructors, as set and clone have to have 5 functions each for each type of cone.

Regards,

Marc

First you should reduce your code by eliminating code duplication.  You may already have noticed that it would be nice to have a function (or better an operator) to compare vectors.  The reason for not defining such an operator in cicommon.h is that the core interface is designed to be used as a library in other programs.  These programs usually already have a powerful math library I don't want to mess up.

Don't rely on class name being "Cone", but ClassName() has to be Type().  In fact the pointers have to be the same.  This is what you test with ==, otherwise you had to use strcmp() for example.  Where these pointers point to isn't important for the usage of the library but a meaningful string makes the debugging or logging easier.

You should tests the standard constructor for creating a valid cone.

To reduce the confusion in coding style the tests of the primitives are using the style described in brlcad/trunk/HACKING.  I.e. no need to use capital letters in file names.  And you should review the spaces in your patch.

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.

Code duplication: Should I create a function that compares two Vector3Ds as a local function?

Pointer comparison : Can I get a hint as to how that is done? This is my second day of working with OOP, have not worked with pointers before.

Valid Cone : There is an "IsValid" test that tests exactly that, if the created cone passes the IsValid() function.

Spaces : Andrei told me via IRC that when I want to comment on what something does, rather than going over the 120character/line mark I should put it as a comment above the function, and then an empty line for visibility. Is that not the case? My coding style fits what you told me to read in the HACKING file ( i.e braces in ifs, braces in functions, etc. ), so I do not see the issue.

The function names are capitalized when declared, do I not need to call them as they are?

The work on this task is ready to be reviewed.

Code duplication: Exactly, a function or the == and != operator:

class Foo;
bool operator==(const Foo left, const Foo right) {
...
}
Foo a, b;
if (a == b) {

Pointer comparison: You should know that == compares the pointers, not the values they point to.  The return type of both ClassName() and Type() is const char* i.e. a pointer type!  BTW, this logic is C not C++.

Valid cone: First, you don't test the result of the standard constructor but the result of the Set() method called afterwards.  And you test only one cone for validity, especially not the one you add to the database.

Spaces:  You really should review your indents.  There is a section about them in HACKING.  (I haven't mentioned your comments.)

I wrote "file names" in my comment.

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.

Coding style : Braces for functions go on the next line, braces for everything else, same line. Function returns go on a separate line for visibility. All functions have a comment saying which test scenario they are for, main is also documented. All the functions are static, to be used only in this file. Function names are mixed lowercase and uppercase, for when new words begin. Local variables are decalred near their usage. Variables have a meaningful name, easy to understand. C++ Stream I/O is used instead of C standard I/O.

File Name : Fixed, now not capitalized.

Valid Cone : Cones are now validated before being added to the database, there is no way an invalid cone can pass the test.

Pointer Comparison : Now used strcmp() to compare the two char* outputs.

Code duplication : Added a static bool function which compares two Vector3Ds, was really useful in cleaning the code, now everything looks way better.

Hope this one is okay :)

Regards,

Marc

The work on this task is ready to be reviewed.

The comparison of ClassName() and Type() with == was OK because you have to compare the pointers (as I wrote in my  first comment here).  "Where these pointers point to isn't important for the usage of the library ..."

My other comments regarding pointer comparison explains the difference between a pointer and the data the pointer points to and how this can be compared (because you asked for it).  Here the comparison of the address in the memory where the pointer points to is sufficient.

Have you run the Test?

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.

Changed the comparison back, you are correct.

I did not run the test as I am on a newly installed distro and have some issues with cmake/make that do not allow me to, unfortunately..

Regards,

Marc

The work on this task is ready to be reviewed.

 Marc,

you need to fix your system so you can build. What's stopping you from doing so ? Ask for help !

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.

There seems to be missing a cmake file to actually "make test" on, so I'll reinstall in the morning, however that will take a lot of time and I'd rather do it in parallel while working on another task.

Is my code okay? From my POV this is a long way I've come working on OOP and I'm really proud of it, but I've been working a lot on it and would be glad to know if it's done.

The work on this task is ready to be reviewed.

Congratulations, this task has been completed successfully.

Marc, this is looking better.  There are still a handful of issues that'll need to be fixed before this can be used, but it's a good start and indeed you have learned a lot.  If/As you take on more unit tests, those other issues can be addressed as continual improvements.

Note that as you complete more and more coding patches, we will slowly expect more and more improvements to your quality and conformance with our coding guidelines.  Basically, we'll keep raising the bar and hope you will strive to meet the challenge. ;)

When you get to the point that there is no more room for improvement in your patches (both logic and style), you'll be ready to commit directly.