From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.5-pre1 (2020-06-20) on ip-172-31-74-118.ec2.internal X-Spam-Level: X-Spam-Status: No, score=-1.9 required=3.0 tests=BAYES_00 autolearn=ham autolearn_force=no version=3.4.5-pre1 Path: eternal-september.org!reader02.eternal-september.org!news.swapon.de!fu-berlin.de!uni-berlin.de!individual.net!not-for-mail From: Niklas Holsti Newsgroups: comp.lang.ada Subject: Re: algorithm, two implementations that act the same, same type etc Date: Wed, 17 Feb 2021 19:10:54 +0200 Organization: Tidorum Ltd Message-ID: References: <0997fa9b-c607-4c61-a8cb-3d3dc3ce3904n@googlegroups.com> <8dc52127-3627-4bc1-b4d5-31935c3a4a37n@googlegroups.com> <67a2f69e-669a-4b19-bba2-af025b8a440cn@googlegroups.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Trace: individual.net y70K/EGNzPIwxyZSJ8Q//wz1iMPE9aBUObG/ccu/2a2JGt+5Ov Cancel-Lock: sha1:NU1AFTRuOwnozzQKn4fMbBEdAvI= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 In-Reply-To: <67a2f69e-669a-4b19-bba2-af025b8a440cn@googlegroups.com> Content-Language: en-US Xref: reader02.eternal-september.org comp.lang.ada:61366 List-Id: On 2021-02-17 14:14, Mehdi Saada wrote: >>> I'll refactor stuff and you'll tell if that complies with your standard. We won't know who's standard you mean if you don't quote something from the person or name the person. I'll assume it's me, so I'll review your code from my point of view. I won't put in softening polite words everywhere, but don't be offended by my criticism, please :-) > I hope that's better. > ------------------ > body part of "p_expression_a.a_moi") > function Computation (V: in T_Vect_Token) return integer is separate; > function Calcul(V: in T_Vect_Token) return Integer renames Computation; The first thing a reader would ask themselves is "why that renaming"? That would be worth a comment. My practice is to describe the role and meaning of a subprogram in connection with that subprogram's declaration, which you haven't done here. Even a one-liner like "The result of evaluating the post-fix expression V" would be illuminating. > ---------------------- > separate (P_Expression.A_Moi) > function Computation ( V : in T_Vect_Token) return Integer is > > package P_stack is new P_Pile_4(T_Elem => Integer, > Max_Pile => 80); > > -- call on stack package I don't see what the comment above brings to the table. I can see from the code that something related to a stack package is being done; I don't need a comment to tell me that. The comment should tell me something that I cannot see directly from the Ada code. Perhaps that the stack in question will hold operand and result values (which "T_Elem => Integer" does not tell me, because Integer is such a general type). I also make a rule that comments should be written in normal prose style, as full and grammatical sentences, with an initial capital letter and a final period in each sentence. > subtype T_stack is P_stack.T_Pile; > subtype T_Token_Operand is T_Token_Operande; > subtype T_Token_Operator is T_Token_Operateur; > procedure Stack_Uppon (A_Stack : in out T_stack; Elem: Integer) > renames P_stack.Empiler; > procedure Pop (A_Stack: in out T_STACK; Elem: out Integer) > renames P_stack.Depiler; > function Is_Stack_Empty (A_Stack: T_stack) return Boolean > renames P_stack.Pile_Vide; > Exception_Wrong_Token : exception renames P_Expression.Exc_Erreur; If those renames are only to translate French to English, you took my comment about French too seriously. There's nothing bad with using other languages for identifiers and comments -- I sometimes use Finnish -- but if you then post the code in an English-language forum like comp.lang.ada, you can expect some incomprehension if you don't explain the names in your message, for example saying "Empiler" = "push" and "Depiler" = "pop". If you prefer French discussions, you can post on fr.comp.lang.ada. I once took part in an ESA project by a French company that was building an ESA satellite based on an earlier French national (CNES) satellite. All code and documentation for the earlier satellite was in French, but the ESA satellite required English documentation, and also some changes to the SW. So, for a while the French "method" for marking changes in the SW requirements specification document was that all old text was in French, all changes and new text in English -- even when changing only part of a sentence. And of course all acronyms came in two forms, like OTAN (French) = NATO (English). This method somewhat limited the staff we could assign to the project. > -- renamings for conveniance, avoids usage of "use" The renames added 10 lines to the code, with possible errors in them, replacing one line for "use", or a few cases of qualification by package name. Not a win, IMO. You could also have avoided "use" by saying "type T_Stack is new P_Stack.T_Pile" (which would inherit all operations of T_Pile) instead of using a subtype. Or you could say "use type T_Stack" or "use type P_Stack.T_Pile" with the same effect. My personal rule for "use" is to limit it to scopes that are so small (few lines) that the "use" is always visible on the screen when I am working on the code affected by it. And even so only when it really makes the code significantly easier to read. Note that avoiding "use " is made much easier by hierarchical packages: code in package Foo.Bar.Joe sees everything declared in packages Foo and Foo.Bar without having to qualify them or saying "use Foo" or "use Foo.Bar". > > Stack_for_Operands : T_stack(40); > Operand_1, Operand_2: Integer; Your identifiers are clear enough to almost avoid the need for comments per object. But the penalty for that is that the identifiers are long, especially the first one. I would probably have written: Stack : T_Stack (40); -- Holds operands and computation results. (By the way, what is the "40" for here? To set the maximum stack size? But isn't that given by "Max_Pile => 80" in the generic instantiation?) > begin > > -- Loop runs through the token vector, if it's an operand it's stacked, > -- if it's an operator the last two operands stacked are recovered and the computation made, > -- but in the inverse order of the recovered operands, or "/" and "-" would misbehave. > > for I in V'Range loop > > -- Tag comparisons to choose decision Some people write comments before the subject code, some people write comments after the subject code. I do both, but for different purposes, and I write the comment differently to separate the two cases. When I want to write a comment that applies to a long piece of code, for example several statements or declarations (as in the earlier renamings) I put the comment before the code, surround it with blank lines, and end it with a colon. I would replace the above comment with: -- See if V(I) is an operand or an operator: The comment does not have to say "Tag comparisons"; I can directly see from the code that it is comparing tags. The phrase "to choose decision" is redundant; any comparison is done to choose or decide something. The important thing is what is _specific_ to these comparisons and decisions, which here is to separate operands from operators. The common example of a useless comment is: I := I + 1; -- Add one to I. Don't emulate it. When I want to write a comment that applies to a short piece of code, such as a single declaration or statement, I put the comment after the code and ensure that there are no blank lines between the code and the comment. I usually add a null comment between the code and the comment, to avoid the "blurring" effect: Result : Integer; -- -- The result of the computation. > if V(I).all'Tag = T_Token_Operand'Tag then My practice is to write a comment after every "then" and "else" to explain the situation. Here I would have written: if V(I).all'Tag = T_Token_Operand'Tag then -- This is an operand, so we push its value on the stack: > Stack_Uppon(Stack_for_operands,T_Token_Operand(v(I).all).Get_Elem); The usual English name for pushing something on a stack is "Push", just a note. > -- Get the value to stack If that comment applies to the "Stack_Uppon" statement, it should be indented as much as that statement. And I would add a null comment between the statement and the comment to show that the comment applies to the preceding statement. > elsif V(I).all'Tag = T_Token_Operateur'Tag then And here I would write, after the "then": -- This is an operator, so we apply it to the two top operands -- from the stack, and replace the operands with the result: > Pop(Stack_for_operands,Operand_2); > Pop(Stack_for_operands,Operand_1); > > -- decision over which operator to apply. > > case T_Token_Operateur(v(I).all).L_Operateur is > when '*' => Stack_Uppon(Stack_for_operands,Operand_1 * Operand_2); > when '-' => Stack_Uppon(Stack_for_operands,Operand_1 - Operand_2); > when '+' => Stack_Uppon(Stack_for_operands,Operand_1 + Operand_2); > when '/' => Stack_Uppon(Stack_for_operands,Operand_1 / Operand_2); > end case; > > else raise P_Expression.Exc_Erreur > with "wrong token inserted please review previous steps"; > > -- if any wrong token (parentheses, terminator) has still crept in > -- from the previous steps. Or if the vector is invalid and it has not been seen, A further rule: "else" should almost always be followed by a line break and a comment to explain the case. So here I would write: else -- This is neither an operand nor an operator, which means -- that the input V is in error. raise ... > > end if; > end loop; > > declare > Result: INTEGER renames Operand_1; Why use a renaming here? It buys you nothing; it probably won't even save a few bits of memory. All it does is change the value of Operand_1 in a sneaky manner. > -- clarity rules ! That comment is almost masterfully unclear, similar to "this statement is a lie". I give you credit for localizing the declaration to a declare block. However, why were you not consistent and localized Operand_1 and Operand_2 also? Anyway, for a short subprogram like this, I think using declare blocks is superfluous because the declarations are close to the code anyway. > begin > Pop(A_Stack => Stack_for_Operands, > Elem => Result); > pragma Assert (Is_Stack_Empty(Stack_for_operands) > and (P_Expression.Calcul(V) = Result)); If you use a complex Boolean expression in an assertion, and the assertion fails, you won't know which part of the Boolean expression was False. Better to separate into two assertions, one for the stack being empty, and one for checking the result. > return Result; > end; > exception > when P_stack.Exc_Pile_Vide => Put_Line("Invalidity not recognized by previous subroutine"); > return 0; > when E: others => Put_Line(Exception_Information(E)); return 0; > end Computation; For exception handlers I usually follow the same rule as for "then" and "else": ensure a line break after "=>" and follow it with a comment that explains why this exception might happen (if it isn't evident). For example: when P_Stack.Exc_Pile_Vide => -- The expression V is ill-formed; it tries to apply an operation -- to a stack that is empty or contains only one operand, or the -- entire expression is null and produces nothing. In conclusion, if this were a real code review in a project, I would say "please correct accordingly and submit for re-review". No hurt feelings, I hope.