Custom Apex rule questions

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Custom Apex rule questions

Caleb Knox
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel
Reply | Threaded
Open this post in threaded view
|

Re: Custom Apex rule questions

Juan Martín Sotuyo Dodero

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. 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. 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 <[hidden email]> 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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel



------------------------------------------------------------------------------
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel
Reply | Threaded
Open this post in threaded view
|

Re: Custom Apex rule questions

Caleb Knox
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 <[hidden email]> 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. 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. 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 <[hidden email]> 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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel





--
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel
Reply | Threaded
Open this post in threaded view
|

Re: Custom Apex rule questions

Caleb Knox

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 <[hidden email]> 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 <[hidden email]> 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. 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. 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 <[hidden email]> 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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel





--
Caleb Knox

Endeveran



--
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel
Reply | Threaded
Open this post in threaded view
|

Re: Custom Apex rule questions

Caleb Knox

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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. 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. 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 <[hidden email]> 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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel





--
Caleb Knox

Endeveran



--
Caleb Knox

Endeveran



--
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel
Reply | Threaded
Open this post in threaded view
|

Re: Custom Apex rule questions

Caleb Knox

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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. 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. 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 <[hidden email]> 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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel





--
Caleb Knox

Endeveran



--
Caleb Knox

Endeveran



--
Caleb Knox

Endeveran



--
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel
Reply | Threaded
Open this post in threaded view
|

Re: Custom Apex rule questions

Juan Martín Sotuyo Dodero

ApexOptimizeSoqlRule:

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?

Isn't this the "right" way to do it? Why do you want to look for it if it's ok?

Actually, I am not sure how to do this, since ASTVariableDeclarationStatements doesn't have a getCanonicalQuery() method, or anything like that.

I think you got this mixed up. The getCanonicalQuery() method is for the SOQL's raw node, but you don't really care about it for this rule. You just want to check there the variable to which the SOQL is assigned to is used.

ApexReadabilityRule:

Triggers in Salesforce should not have any logic. The trigger should call a handler.

Given an example of what would be right / wrong would certainly help. I'd not even be able to recognize a trigger from a class at this moment.

ApexSingleSoqlResultRule

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.

I'd have to check, but you can probably get it from the raw node from Apex Jorje (node.getNode())... Not sure though

On Fri, Jul 14, 2017 at 2:33 PM, Caleb Knox <[hidden email]> wrote:

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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. 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. 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 <[hidden email]> 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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel





--
Caleb Knox

Endeveran



--
Caleb Knox

Endeveran



--
Caleb Knox

Endeveran



--
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/pmd-devel