"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