Re: [boost] [Review][PQS] Physical Quantities System review (May 31through June 9)

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2006-06-03 14:18:50

"fred bertsch" wrote:
> The review of Andy Little's Physical Quantities System begins today,

I do abstain in this voting.

The documentation could be improved a lot.
I didn't spotted problems in the code, the tests
compile (with tiny fix [13]) and the usability of the
library looks within reach of normal developer.

/Pavel

_______________________________________________
1. The name "pqs" should be changed to
   something a little bit more descriptive.

   "unit" feels quite handy.

_______________________________________________
2. intro.html: I find some terms as "t1_quantity"
   rather annoying. What they mean? How do these
   names help novice to understand the library
   better?

   I would recommend to avoid the t1, t2, ...
   nomenclature. I know about few projects
   named tX, it is better to avoid such associations.

   The funny "physical_quantity" uses underscore
   intentionally? Shoudn't italics or different font
   be used?

   What may be implemented in the future should
   be moved out of introduction. A novice will
   have enough of troubles to grasp what's
   available now.

   Marketing ala "Increases programmer satisfaction
   and enhances productivity" should be left out.
   It is up to the user to decide usability
   of the library.

_______________________________________________
3. definitions.html: ERD abbrev should be spelled fully.
   Smaller and more focused parent - child diagrams
   could be added for every term, instead of pointing
   to one huge and hard to grasp quickly graph.

   The detailed description of terms should start from
   the basics and the most simple and continue toward
   more complex terms - the lexicographic ordering
   is not that helpful. The ordered list on top is OK.

   Every single term should have example or link
   to example. It is too information dense as it
   is now and it doesn't give enough of clues to novices.

   I was not able to understand the last paragraph
   in base_dimension section.

   It shlould be always spelled what every term means
   in C++: class, typedef, constant, variable, concept?

   In "dimension": what is
    "show access to dimension typedef in t1_quantity"???

   I am not sure whether "incoherent_unit" is
   the best term - perhaps traditional or historical
   or specialised.

   C.G.S. is not linked.

_______________________________________________
4. There should be "How to..." for most importants
   use of the library. Acknowledgements should be
   the last page. Not many people will have stamina
   to read through all the stuff to find out what
   is the library good for.

_______________________________________________
5. A table listing all namespaces and
   all relevant typedefs inside, all hyperlinked,
   is missing. The information may be present
   but not in single location and compact form.

   Instead of lot of free text I would much more
   prefere tables and examples.

_______________________________________________
6. rationale.html: instead of headings as
   "clarity" a technical description should
   be used, e.g. "replacing untyped calculation
   with typed" or so.

   Dtto the "More enjoyable programming" and so on.

   The example in "More enjoyable programming"
   doesn't say explicitly what useful happens inside.
   Even after several readings I am not able
   to understand what one should expect to learn here.

   "Code integration" section is empty of information.
   And what gets integrated where?

_______________________________________________
7. Not only examples that show how somethink works
   but also examples that show what fails and
   what kind of mistakes the library prevents
   should be included.
   Such examples could easily get the most
   useful resource for the learning process.

_______________________________________________
8. All examples in libs/pqs/examples should
   be linked from the main documentation with
   explicit note what one should notice in each of them.

_______________________________________________
9. library_interface.html:

   The text like "...the complexities are hidden
   behind a simple interface and its not necessary
   to know the full details of the type to make
   good use of it" >> /dev/null

   Path names as
     #include <boost/pqs/t1_quantity/types/length.hpp>

   - I am against showing history related details (t1, t2, ..)
   to the user. One wants to used the library w/o
   hassle, not to deal with "t1".

   "Stream output" section - the exmple should mention what
   is actual output as a comment next to the << operation.
   All examples should be consistent in style.

   I am not sure whether stream I/O formatting
   really belongs to the library - there could
   be large issues with I18N. There should
   be separate page in the documentation detailing
   current use and what one could do to use own
   formatting.

   Examples: I would like to see longer names for
   variables than "t", "t1", "m1", "v1".
   Especially the "t1" irks me in he context.

   The terms as "dimensionful" in sectin names
   should be hyperlinked. Dimension-full feels
   more readable.

   "Addition and subtraction" section: the heavy
   hyperlinking over and over makes the text practically
   unreadable. I really recommend to get rid
   as much of free text as possible and replace
   it with more structured content (including examples).

   "Multiplication, division" - small examples should
   follow every sentence. It is hard to identify them
   in the large blob bellow.

   "Calculations - performance" - last paragraph is
   about what?

_______________________________________________
10. t1_quantity.html:

   The macros - are the really needed when Boost
   has existing typedefs?

   "Values" section - I am unable to understand
   what it describes and what it could be used for.

   I completely gave up reading the rest of page
   - I do not see structure or some anchor point.

_______________________________________________
11. rest of documentation:

length.html: point (prn) - I guess this means
printing (or better prepress) but does everyone know?

The "acre" here is length or breadth?

voltage.html: abvolt and such curiosities
should be linked to external definition, if only
to verify that the implementation is correct.

Perhaps a link to http://www.unc.edu/~rowlett/units/
would be the best.

physical_constants.html: year or so ago
library by Paul Bristow providing many constants
was on review (and rejected for the time).
If it is still worked on it would be good
to coordinate the content.

extending.html: this should have form of tutorial,
with steps 1., 2., 3. ... and description how to
verify the result.

reference.html: why is Google redirect there?
The CUJ link should be changed to current DDJ.

_______________________________________________
12. Normal headers and "out" headers. Perhaps
    these should be merged together. The code in
    pqs library uses so much of other Boost
    functionality that additional <iostream>
    and <sstream> won't make difference
    on compilation time (it takes seconds to tens
    of seconds to compile any example on my machine with
    Intel 9, the streams alone take less than second).

    IMHO it is worth to make the lib simpler to use.

_______________________________________________
13. t1_quantity/operations/compare.hpp:

The
#include <cmath>

should be replaced with

#include <boost/compatibility/cpp_c_headers/cmath>

which works for Intel C++ plugged
in VC6 IDE with old Dinkumware.

The header
<boost/compatibility/cpp_c_headers/cmath>
has a bug (abs() is missing) but it has
been reported.

_______________________________________________
14. typeof_register.hpp: the warning against
    Typeof changes after admitting to Boost
    should be updated/removed.

_______________________________________________
15. quantity_traits.hpp: the constants
    max_default_named_quantity_tag would look
    better as MaxDefaultNamedQuantityTag.
    It is rather conventional style.

---------
The names in template<typename T, typename S>
could be more expressive.

_______________________________________________
16. Indentation style. The classes would be better
    moved to the very left side (i.e. ignoring
    namespaces). Currently way too many lines
    get splitted in half and the result is harder
    to parse (by humans).

    4 or 8 characters could be gained for lot of the code.

_______________________________________________
17. config.hpp: the BOOST_PQS_SUPPRESS_VC8_ADL_BUG
    should be likely limited to VC8.

--------------
BOOST_PQS_DEFINE_PHYSICAL_CONSTANTS_IN_HEADERS
may have some comment nearby what to do when
compiler doesn't support the feature.

_______________________________________________
18. concept_checking.hpp: if possible style
    of #include should be the same all over:

#include "boost/type_traits/is_arithmetic.hpp"
#include <boost/concept_check.hpp>

------
Quite a large block of commented out code
exists here. It should be used or removed.

------

Perhaps BOOST_STATIC_ASSERT could be used
instead of rolling out own Assert
(just a minor point).

------

The Pair<> could be possibly replaced
by std::pair.

_______________________________________________
19. utility/timer.hpp: this looks as if it should
    be in test/ or examples/ folder.

_______________________________________________
20. The whole pqs/two_d: is this documented
    somewhere or is it start of future work?
    Dtto three_d.

_______________________________________________
21. t1_quantity/t1_quantity.hpp:

public function get_united_value()
returns something from detail sub-namespace.

_______________________________________________
22. t1_quantity/types/capacitance.hpp (and others):

    Types as GF (giga Farad) or even much bigger
    are defined. I am not sure whether in current
    universe such capacitance could exist.
    Perhaps those physically impossible types could
    be pruned out to avoid mistakes caused by typos
    (for example mF and kF are easy to mis-type).

    When someone wants GF then he may add such
    thing manually.

    Or perhaps there could be a macro like
    USE_FULL_EXTENT_FOR_UNITS that enables
    such types.

--------
    And when I am at it: when one uses nF and mF
    mistake is easy to happen and hard to spot.

    I would love to have typedefs with names
    like nano_farad and micro_farad available as well.
    The cost of typing is smaller than searching for
    a bug caused by wrong unit.

_______________________________________________
23. The t1_quantity/types/xxx.hpp should contain
    exact information where in the documentation
    look for recipe how to create new types.

    (And the docs should refer this file.)

_______________________________________________
24. t1_quantity/types/time.hpp:

    I would prefer to spell a type as "week"
    instead of "wk".

_______________________________________________
25. Naming: people may not like long names
    like boost:pqs:xyz.

    Perhaps there could be a macro that,
    given a namespace (i.e. global) and common
    name prefix, will create all typedefs
    where the user likes them. Something as

    BOOST_UNITS_DECLARE_ALL_IN(::, unit_)

    and then "unit_m" would be meters.

_______________________________________________
26. The library provides dimensionless
    units (if I understanding it correctly).

    For many applications these would be the
    most useful ones. There should be page
    dedicated only to this functionality,
    not bothering a read with Farads and inches.

    There should be examples like
     number_of_apples = number_of_oranges; // error
     number_of_fruit = number_of_apples; // OK
   that give enough of clue without need
   to read the whole documentation.

_______________________________________________
27. The documentation should say (perhaps I missed
    it) whether the different unites could be used
    for overloading of functions, with example(s).

_______________________________________________
28. As I read the code, Boost.Serialisation is
    not explicitly supported and won't work by
    out of box. I guess it should be reatively
    easy to add.

_______________________________________________
EOF