[brlcad-tracker] [Google Code-in 2014] New comment on Write CoreInterface unit test #3

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Claimed

I would like to work on this task.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


ch3ck has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Assigned

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


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Feedback


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


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Ready for review

The work on this task is ready to be reviewed.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


brlcad has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Needs More Work

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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


brlcad has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


constructors


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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Ready for review

The work on this task is ready to be reviewed.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Why so much code?


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


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


rossberg has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


None


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. Ie no need to use
capital letters in file names. And you should review the spaces in your
patch.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


rossberg has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Needs More Work

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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


???


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 ( ie 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?


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Ready for review

The work on this task is ready to be reviewed.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


rossberg has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Some Explanations


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* ie 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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


rossberg has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Needs More Work

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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Fixes


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


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Ready for review

The work on this task is ready to be reviewed.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


rossberg has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Some More Clarifications


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?


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


rossberg has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Needs More Work

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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Modified it back


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


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Ready for review

The work on this task is ready to be reviewed.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


popescuandrei has left the following comment at Write CoreInterface unit
test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


None


Marc,

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


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


popescuandrei has left the following comment at Write CoreInterface unit
test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Needs More Work

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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Conclusion


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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


tannousmarc has left the following comment at Write CoreInterface unit test
#3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Ready for review

The work on this task is ready to be reviewed.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


brlcad has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


Task Closed

Congratulations, this task has been completed successfully.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.

[n***@google-melange.appspotmail.com]

Hi,


brlcad has left the following comment at Write CoreInterface unit test #3
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360:


needs more, but good effort


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.


Greetings,
The Google Open Source Programs Team


---
You are receiving this message because you are subscribed to Write
CoreInterface unit test #3.
To stop receiving these messages, go to:
https://www.google-melange.com/gci/task/view/google/gci2014/5900674122383360.