View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0024636 | Open CASCADE | OCCT:Documentation | public | 2014-02-17 20:31 | 2014-05-05 13:38 |
Reporter | kgv | Assigned To | apn | ||
Priority | normal | Severity | integration request | ||
Status | closed | Resolution | fixed | ||
Target Version | 6.7.1 | Fixed in Version | 6.7.1 | ||
Summary | 0024636: Coding Rules - define rules for development of Draw Harness commands | ||||
Description | Existing code base of Draw Harness commands significantly fluctuates in command line syntax, arguments processing, safety checks, output printing. It is suggested to define rules / best practices for development of Draw Harness command to follow common style in future. | ||||
Tags | No tags attached. | ||||
Test case number | Not needed | ||||
|
Dear abv, please review the first version in CR24636 branch. |
|
No remarks, please proofread (and check generating documentation) |
|
Reviewed, corrected grammar, style, formatting and logics. Removed redundancies. |
|
Yuri, thank you for the revision! Please double check a few points I noticed: - it seems that some words are lost in the text. For instance, at line 36 probably 'look' is missing before 'at the existing names of toolkits'. There are more examples in the text. - if use of ... and other HTML tags is correctly handled by PDF generator |
|
Dear Yuri, in general it is bad idea to include changes unrelated to the issue description. Such changes should be done within dedicated issue. > until all major compliers on all supported platforms Could you please fix misprint in addition, while you here (compliers -> compilers)? > should be optional (avoided with appropriate preprocessor checks) _avoided_ is inappropriate word for this message (originally used "escaped" makes sense here). -If such iteration is used, make sure that the result of the algorithm does not depend on order. +If such iteration is used, make sure that the result of the algorithm does not depend on the order if iterated items. _if_ iterated -> _of_ iterated? ~~~~~{.cpp} -Standard_Integer awidthofbox; // this is bad +Standard_Integer widthofbox; // this is bad Standard_Integer width_of_box; // this is bad -Standard_Integer aWidthOfBox; // this is OK +Standard_Integer WidthOfBox; // this is OK ~~~~~ What a hell? Rule has been corrupted! Please don't touch code samples. -It is preferred to name public methods from upper case, while protected and private methods from low case. +It is preferred to start names public methods from an upper case character and to start names of protected and private methods from a lower case character. start names _OF_ public methods? +The name of a local variable name should be distinguishable from the name of a function parameter, a class member variable and a global variable. + namename +Avoid dummy names, such as I, j, k Please fix pre-existed misprint here - I should be lowercase. -In all sources try not to exceed 120 characters limit of line length. + +Try to stay within the limit of 120 characters per line in all sources . redundant space before period. -Single-line conditional operator (if, while, for etc.) can be written without brackets on the following line. + +Single-line conditional operators (*if, while, for, etc.) can be written without brackets on the following line. broken formatting? + +This rule improves readability, allows detecting useless multiple header inclusions and makes 3rd-party dependencies clearly visible. +Following these rules is important for good comprehension of the comments. Moreover, this approach allows automatically generating user-oriented documentation directly from the commented sources. + double space between words. |
|
Thanks for the reviewing. I should have rechecked my own changes. One question: " > should be optional (avoided with appropriate preprocessor checks) _avoided_ is inappropriate word for this message (originally used "escaped" makes sense here). " Meaning of "escape" (in computer science) = "To interrupt a command, exit a program, or change levels within a program by using a key, combination of keys, or key sequence." So all optional "compiler-specific features" can be interrupted with "appropriate preprocessor checks". Is it what you wish to say? |
|
> Meaning of "escape" (in computer science) = "To interrupt a command, exit a program, or change levels within a program by using a key, combination of keys, or key sequence." "Escaping" might has meaning "экранирование" in development (see "escape character" as most often use case). The whole message might be better written as: > should be optional (used only within appropriate preprocessor checks) |
|
Remarks taken into account. Correct PDF generation checked. |
|
Dear bugmaster, please integrate the patch. Please make sure that final commit message will not include triple copy of bug name. |
Date Modified | Username | Field | Change |
---|---|---|---|
2014-02-17 20:31 | kgv | New Issue | |
2014-02-17 20:31 | kgv | Assigned To | => abv |
2014-02-17 20:31 | kgv | Assigned To | abv => kgv |
2014-02-17 23:38 | kgv | Note Added: 0027937 | |
2014-02-17 23:38 | kgv | Assigned To | kgv => abv |
2014-02-17 23:38 | kgv | Status | new => resolved |
2014-02-18 08:49 |
|
Note Added: 0027939 | |
2014-02-18 08:49 |
|
Assigned To | abv => ysn |
2014-02-18 08:49 |
|
Status | resolved => reviewed |
2014-02-20 19:52 |
|
Note Added: 0027988 | |
2014-02-20 20:10 |
|
Note Added: 0027989 | |
2014-02-20 21:53 | kgv | Note Added: 0027990 | |
2014-02-21 11:52 |
|
Note Added: 0027991 | |
2014-02-21 12:02 | kgv | Note Added: 0027992 | |
2014-02-21 12:02 | kgv | Note Edited: 0027992 | |
2014-02-21 16:26 |
|
Note Added: 0027993 | |
2014-02-21 16:49 | kgv | Note Added: 0027994 | |
2014-02-21 16:49 | kgv | Assigned To | ysn => bugmaster |
2014-02-25 16:15 | apn | Test case number | => Not needed |
2014-02-25 16:15 | apn | Status | reviewed => tested |
2014-03-03 13:25 | apn | Changeset attached | => occt master 49663e13 |
2014-03-03 13:25 | apn | Assigned To | bugmaster => apn |
2014-03-03 13:25 | apn | Status | tested => verified |
2014-03-03 13:25 | apn | Resolution | open => fixed |
2014-03-04 19:35 | kgv | Relationship added | related to 0024685 |
2014-03-05 14:28 | kgv | Relationship replaced | parent of 0024685 |
2014-05-05 13:33 |
|
Status | verified => closed |
2014-05-05 13:38 |
|
Fixed in Version | => 6.7.1 |