Re: [pmd-devel] Custom Apex rule questions

[Caleb K. <cal...@en...>]

ApexSingleSoqlResultRule

I am checking ASTSoqlExpression nodes. If their parent nodes include a
>> try/catch, no violation. Else, add violation.
>>
> 2 things here. I believe you already thought about this, but the try may
> not be a direct parent but some other ancestor of the node, and you have to
> actually check if the exception being caught is a QueryException
>
I can't seem to figure out how to check for a QueryException specifically
as in the TryCatchFinallyBlockStatement node, there isn't a way to access
what's inside the catch() argument, from what I can tell using PMD Designer.


On Fri, Jul 14, 2017 at 10:46 AM, Caleb Knox <cal...@en...>
wrote:

>
> ApexOptimizeSoqlRule:
>>
>> looking at ASTSoqlExpression nodes and checking their parents for a
>>> List<> declaration
>>
>> This is actually good. Once you get that List<> declaration, you need to
>> check if it is being used in a foreach and if so flag it.
>>
> Actually, I am not sure how to do this, since
> ASTVariableDeclarationStatements doesn't have a getCanonicalQuery()
> method, or anything like that.
>
>
> On Fri, Jul 14, 2017 at 10:15 AM, Caleb Knox <cal...@en...>
> wrote:
>
>> ApexOptimizeSoqlRule:
>>
>> I was planning on looking at ASTForEachStatement nodes, checking the
>>> children for an ASTSoqlExpression node
>>>
>> I’m not sure I understand this… as far as I can tell, this is actually
>>> the desired code, so there is no point ins searching for it (you only want
>>> to add warnings for violations).
>>>
>> So, what I mean is that I'd like to check the ForEach to see if there is
>> a Soql expression within its declaration, because that's a direct child
>> node. Is that wrong?
>>
>>
>> ApexReadabilityRule:
>>
>>
>>>    - Non trigger framework code exists in triggers => add to violation
>>>    count
>>>
>>> I’m not sure what this would be. I’m not an Apex developer. Given proper
>>> samples I may assist you non the lest given I do know about it’s AST.
>>>
>> Triggers in Salesforce should not have any logic. The trigger should call
>> a handler.
>>
>> Thanks.
>>
>>
>> On Fri, Jul 14, 2017 at 9:14 AM, Caleb Knox <cal...@en...>
>> wrote:
>>
>>> Thanks for the insight. I'll get back to you about the ApexOptimizeSoql
>>> if I don't get it right soon.
>>>
>>> I can definitely add these rules to PMD, assuming my boss okays it.
>>>
>>> On Thu, Jul 13, 2017 at 6:48 PM, Juan Martín Sotuyo Dodero <
>>> jua...@gm...> wrote:
>>>
>>>> Hi Caleb,
>>>>
>>>> I’m glad to see you are making progress. Are you planning to contribute
>>>> these rules back to PMD? If so, it will be easier / we will be far more
>>>> willing to help you out. Just submit a PR for each in such case and we will
>>>> comment directly on your code.
>>>>
>>>> In the meantime, let me go in order:
>>>> ApexOptimizeSoqlRule:
>>>>
>>>> I was planning on looking at ASTForEachStatement nodes, checking the
>>>> children for an ASTSoqlExpression node
>>>>
>>>> I’m not sure I understand this… as far as I can tell, this is actually
>>>> the desired code, so there is no point ins searching for it (you only want
>>>> to add warnings for violations).
>>>>
>>>> looking at ASTSoqlExpression nodes and checking their parents for a
>>>> List<> declaration
>>>>
>>>> This is actually good. Once you get that List<> declaration, you need
>>>> to check if it is being used in a foreach and if so flag it.
>>>> ApexSingleSoqlResultRule
>>>>
>>>> I am checking ASTSoqlExpression nodes. If their parent nodes include a
>>>> try/catch, no violation. Else, add violation.
>>>>
>>>> 2 things here. I believe you already thought about this, but the try
>>>> may not be a direct parent but some other ancestor of the node, and you
>>>> have to actually check if the exception being caught is a
>>>> QueryException
>>>>
>>>> I also check the ASTSoqlExpression for a LIMIT 1 since that’s how
>>>> people avoid the QueryException.
>>>>
>>>> This is a must to avoid flagging all SOQL. Given ASTSoqlExpression node
>>>> you can do node.getNode().getCanonicalQuery() to obtain the contents
>>>> of the expression being used.
>>>> ApexCommentRule
>>>>
>>>> can I just use a Java rule, like CommentRequiredRule?
>>>>
>>>> No you can’t. The Java rules use the Java Lexer and Parser, that only
>>>> recognize Java code. You need Apex specific rules to do this.
>>>>
>>>> Comments are not part of the AST, so doing this will not be trivial.
>>>> I’m not sure the Jorje parser provides access to comments at all, I’d have
>>>> to check the APIs and do some tests…
>>>> ApexReadabilityRule
>>>>
>>>>
>>>>    - Lines of code in a method exceed 50 => add to violation count
>>>>
>>>> This is already implemented as apex-complexity/NcssMethodCount
>>>> <https://pmd.github.io/pmd-5.8.1/pmd-apex/rules/apex/complexity.html#NcssMethodCount>.
>>>> The default is 40 lines (without comments), but can be configured
>>>>
>>>>
>>>>    - Nested if statements exceed more than 3 => add to violation count
>>>>
>>>> This is already implemented as apex-complexity/AvoidDeeplyNestedIfStmts
>>>> <https://pmd.github.io/pmd-5.8.1/pmd-apex/rules/apex/complexity.html#AvoidDeeplyNestedIfStmts>.
>>>> The default limit is exactly 3, you would have to set it to 4 to allow up
>>>> to 3.
>>>>
>>>>
>>>>    - Non trigger framework code exists in triggers => add to violation
>>>>    count
>>>>
>>>> I’m not sure what this would be. I’m not an Apex developer. Given
>>>> proper samples I may assist you non the lest given I do know about it’s AST.
>>>>
>>>> Regards,
>>>> ​
>>>>
>>>> On Thu, Jul 13, 2017 at 6:38 PM, Caleb Knox <cal...@en...>
>>>> wrote:
>>>>
>>>>> Hi there folks,
>>>>>
>>>>> I am trying to implement a few more rules for the project I've emailed
>>>>> about before, before we send it off to our customers. This is working with
>>>>> Salesforce/Apex classes.
>>>>>
>>>>> Here they are:
>>>>>
>>>>>
>>>>>
>>>>> *ApexOptimizeSoqlRule*
>>>>> Example:
>>>>>
>>>>> List<Account> acct = […];
>>>>> for (Account a : acct ) {
>>>>>
>>>>> }
>>>>>
>>>>> // add violation
>>>>>
>>>>> -------------------------------------------
>>>>>
>>>>> // optimized version
>>>>>
>>>>> for (Account acct : [Select … ] ) {
>>>>>
>>>>> }
>>>>>
>>>>> // all good
>>>>>
>>>>> For this rule, I was planning on looking at ASTForEachStatement nodes,
>>>>> checking the children for an ASTSoqlExpression node, as well as looking at
>>>>> ASTSoqlExpression nodes and checking their parents for a List<>
>>>>> declaration. However, what I have is not working, so I am wondering if my
>>>>> basic way of thinking about this rule is wrong, and if I should do
>>>>> something else.
>>>>>
>>>>>
>>>>>
>>>>> *ApexSingleSoqlResultRule*
>>>>> Example:
>>>>>
>>>>> Account acct = [Select Id From Account Where Name = ‘abc’ ];
>>>>>
>>>>> // Get QueryException back if 0 or duplicate
>>>>>
>>>>> // People try to avoid by doing:
>>>>>
>>>>> List<Account> acct = [Select … LIMIT 1]; // bad practice to avoid
>>>>> dupes etc --> add violation
>>>>>
>>>>> if (acct.size() > 0 )
>>>>>
>>>>> [...]
>>>>>
>>>>>
>>>>> --------------------------------------------
>>>>>
>>>>> // Best practice is:
>>>>>
>>>>> try {
>>>>> Account acct = […];
>>>>> } catch (QueryException q){
>>>>>
>>>>> }
>>>>>
>>>>> For this rule, I am checking ASTSoqlExpression nodes. If their parent
>>>>> nodes include a try/catch, no violation. Else, add violation.
>>>>>
>>>>> I also check the ASTSoqlExpression for a LIMIT 1 since that's how
>>>>> people avoid the QueryException. But, I don't quite know how to do that.
>>>>>
>>>>> Maybe something like this?
>>>>>
>>>>> if(node.getNode().getQuery().toLowerCase().contains("list") &&
>>>>>                 node.getNode().getQuery().toLowerCase().contains("limit
>>>>> 1"))
>>>>>
>>>>>
>>>>> *ApexCommentRule*
>>>>>
>>>>> The last two rules are related to readability. The first one looks at
>>>>> the following:
>>>>>
>>>>>    - No class or method comments => add to violation count
>>>>>
>>>>>
>>>>> For this, can I just use a Java rule, like CommentRequiredRule?
>>>>>
>>>>> *ApexReadabilityRule*
>>>>>
>>>>> The last one follows these criteria:
>>>>>
>>>>>    - Lines of code in a method exceed 50 => add to violation count
>>>>>    - Nested if statements exceed more than 3  => add to violation
>>>>>    count
>>>>>    - Non trigger framework code  exists in triggers  => add to
>>>>>    violation count
>>>>>
>>>>>
>>>>> I'm not sure how to do this last one.
>>>>>
>>>>> If you have any insight or feedback, let me know!
>>>>>
>>>>> Thank you.
>>>>>
>>>>> --
>>>>> Caleb Knox
>>>>>
>>>>> Endeveran
>>>>>
>>>>> ------------------------------------------------------------
>>>>> ------------------
>>>>> Check out the vibrant tech community on one of the world's most
>>>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>>>> _______________________________________________
>>>>> Pmd-devel mailing list
>>>>> Pmd...@li...
>>>>> https://lists.sourceforge.net/lists/listinfo/pmd-devel
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Caleb Knox
>>>
>>> Endeveran
>>>
>>
>>
>>
>> --
>> Caleb Knox
>>
>> Endeveran
>>
>
>
>
> --
> Caleb Knox
>
> Endeveran
>



-- 
Caleb Knox

Endeveran