search

indiantarget / quimeraengine

0 stars 0 forks source link

QFixedArray 001 #465

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
One of the parts of the development of QFixedArray class.

Task description:

https://drive.google.com/file/d/0B0FuyHpDsrcfbnZIbzRiUklycWM/edit?usp=sharing

Original issue reported on code.google.com by kinesiscontact@gmail.com on 22 Jan 2014 at 12:49

GoogleCodeExporter commented 9 years ago

Original comment by Lince3D@gmail.com on 22 Jan 2014 at 12:59

GoogleCodeExporter commented 9 years ago

Original comment by Lince3D@gmail.com on 22 Jan 2014 at 1:07

GoogleCodeExporter commented 9 years ago

Original comment by Lince3D@gmail.com on 22 Jan 2014 at 1:08

GoogleCodeExporter commented 9 years ago

Original comment by Lince3D@gmail.com on 22 Jan 2014 at 1:10

GoogleCodeExporter commented 9 years ago

Original comment by Lince3D@gmail.com on 22 Jan 2014 at 1:19

GoogleCodeExporter commented 9 years ago

Original comment by rdelashe...@gmail.com on 14 Mar 2014 at 3:17

GoogleCodeExporter commented 9 years ago

Original comment by rdelashe...@gmail.com on 15 Mar 2014 at 1:56

GoogleCodeExporter commented 9 years ago

Original comment by rdelashe...@gmail.com on 28 Mar 2014 at 9:19

Attachments:

GoogleCodeExporter commented 9 years ago 
Results of the review:

QFixedArray.h
---------------
1-Put together the words "operator" and "[]" please.

2-In the assertions: "the array size" => "the array's size".

3-In GetValue:
---*(T*)((pointer_uint_q)m_pAllocator->GetPointer() + uIndex * sizeof(T)) could 
be simplified to  *((T*)m_pAllocator->GetPointer() + uIndex) , usin pointer 
arithmetic.

4-Documentation of Clone: "fasth" => "fast"

5-Clone: As it happened to the Copy method in the allocator, Clone makes a copy 
of the resident elements to the input array. In this case, naming it CloneTo 
would be.. weird. Please change it so the destination is the output parameter 
and adapt the documentation and parameter name to that behavior. I will try to 
be more specific in future task descriptions.

6-Clone: Do you think it would be better not to allow the method continue if 
the size of both arrays is not the same? It seems to be dangerous.

7-SetValue: The same as 3.

8-SetValue documentation: Specify that the assignment operator will be called 
for the element that is currenly occupying that position.

9-Documentation of GetCapacity: The concept of capacity is not the maximum 
number of elements that can be stored in the structure, it is just the maximum 
that can be stored without a reallocation. The capacity is the amount of memory 
allocated in advance to avoid being allocating memory for every element added.

10-Documentation of IsEmpty: "Returns true if the array is empty" => wouldn't 
it be more natural to say something like "Checks whether the array is empty or 
not"? Then you are not repeating the same in the returns section.

QFixedArray_Test.cpp
-------------------------
11-Please, keep the same order of methods in the tests as in the original 
source code in the future.

12-SetValue_AssertionFailsWhenAttemptsToSetAValueToAPositionGreaterThanArraySize
_Test:
---It should be "GreaterThanOrEqualTo"

13-GetValue_AssertionFailsWhenAttemptsToGetTheValueFromAPositionGreaterThanArray
Size_Test: Same as 12.

14-OperatorArraySubscript_ValueIsSetAtCorrectPosition_Test: It is actually 
checking a side effect of the method, I mean, you are executing the method and 
then using the result for something else, in this case, to assign a new value 
to it. It is not bad, but I though it was worth to comment it.
The same happens to 
OperatorArraySubscript_AssertionFailsWhenAttemptsToSetAValueToAPositionGreaterTh
anArraySize_Test, what is really failing is the Get operation, not the Assign 
operation.

15-OperatorArraySubscript_GetsCorrectValueAtSuppliedPosition_Test:
---Please don't use the method under test to prepare the test, use SetValue 
instead in:
fixedArray[POSITION_TO_SET_NEW_VALUE] = NEW_VALUE

16-Documentation of Clone_ClonedArrayHasSameValuesThanTheOriginArray_Test: 
"check" => "checks"

Original comment by Lince3D@gmail.com on 28 Mar 2014 at 2:08

GoogleCodeExporter commented 9 years ago 
One last comment:

17-The second TODO at operator= have not been replaced with the corresponding 
equivalent code.

Original comment by Lince3D@gmail.com on 28 Mar 2014 at 2:14

GoogleCodeExporter commented 9 years ago 
Patch sent by email.

Original comment by rdelashe...@gmail.com on 29 Mar 2014 at 6:33

GoogleCodeExporter commented 9 years ago 
Results of the review: Correct.

Just change the parameter name of the Clone method by "destinationArray", I 
think it will help to make the direction of the copy clearer.

Original comment by Lince3D@gmail.com on 30 Mar 2014 at 1:55