Things I check when I do a code review

Avatar

Avatar
Coach
Employee
Jörg_Hoh
Employee

Likes

1,134 likes

Total Posts

3,161 posts

Correct reply

1,079 solutions
Top badges earned
Coach
Give back 600
Ignite 5
Ignite 3
Ignite 1
View profile

Avatar
Coach
Employee
Jörg_Hoh
Employee

Likes

1,134 likes

Total Posts

3,161 posts

Correct reply

1,079 solutions
Top badges earned
Coach
Give back 600
Ignite 5
Ignite 3
Ignite 1
View profile
Jörg_Hoh
Employee

30-03-2017

I collected a small list of topics I check when I do a code review. Knowing these items gives you the chance to avoid these pitfalls or to make better decisions 🙂

https://cqdump.wordpress.com/2017/03/28/what-i-check-on-code-reviews/

Enjoy,
Jörg

Replies

Avatar

Avatar
Validate 25
Level 10
smacdonald2008
Level 10

Likes

1,409 likes

Total Posts

12,671 posts

Correct reply

2,278 solutions
Top badges earned
Validate 25
Validate 10
Validate 1
Give back 900
Give back 600
View profile

Avatar
Validate 25
Level 10
smacdonald2008
Level 10

Likes

1,409 likes

Total Posts

12,671 posts

Correct reply

2,278 solutions
Top badges earned
Validate 25
Validate 10
Validate 1
Give back 900
Give back 600
View profile
smacdonald2008
Level 10

30-03-2017

Great community write up! 

Avatar

Avatar
Establish
Community Manager
kautuk_sahni
Community Manager

Likes

1,198 likes

Total Posts

6,383 posts

Correct reply

1,147 solutions
Top badges earned
Establish
Coach
Originator
Contributor 2
Contributor
View profile

Avatar
Establish
Community Manager
kautuk_sahni
Community Manager

Likes

1,198 likes

Total Posts

6,383 posts

Correct reply

1,147 solutions
Top badges earned
Establish
Coach
Originator
Contributor 2
Contributor
View profile
kautuk_sahni
Community Manager

30-03-2017

Hi Jörg

This is really nice community work.

I have also pinned the post in the community so that it remains on top.

I will will go through the post and i would definitely share my comments on this.

 

PS: In future, whenever you would like to share good content with community just let me or Scott know  about it, we will pin the post.

~kautuk

Avatar

Avatar
Springboard
Level 3
Sh1ju
Level 3

Likes

13 likes

Total Posts

56 posts

Correct reply

4 solutions
Top badges earned
Springboard
Validate 1
Establish
Ignite 1
Give Back 5
View profile

Avatar
Springboard
Level 3
Sh1ju
Level 3

Likes

13 likes

Total Posts

56 posts

Correct reply

4 solutions
Top badges earned
Springboard
Validate 1
Establish
Ignite 1
Give Back 5
View profile
Sh1ju
Level 3

31-03-2017

Nice work and very useful

Avatar

Avatar
Validate 1
Level 3
daniel_henriqu1
Level 3

Likes

15 likes

Total Posts

56 posts

Correct reply

10 solutions
Top badges earned
Validate 1
Boost 5
Boost 3
Boost 10
Boost 1
View profile

Avatar
Validate 1
Level 3
daniel_henriqu1
Level 3

Likes

15 likes

Total Posts

56 posts

Correct reply

10 solutions
Top badges earned
Validate 1
Boost 5
Boost 3
Boost 10
Boost 1
View profile
daniel_henriqu1
Level 3

31-03-2017

Nice article, Jörg!

Regarding the item 3, how about using Lamba expressions to ensure that a ResourceResolver/Session will remain opened only within a limited scope?

Something similar to Groovy withReader/withWriter.

Regards,

Daniel.

Avatar

Avatar
Coach
Employee
Jörg_Hoh
Employee

Likes

1,134 likes

Total Posts

3,161 posts

Correct reply

1,079 solutions
Top badges earned
Coach
Give back 600
Ignite 5
Ignite 3
Ignite 1
View profile

Avatar
Coach
Employee
Jörg_Hoh
Employee

Likes

1,134 likes

Total Posts

3,161 posts

Correct reply

1,079 solutions
Top badges earned
Coach
Give back 600
Ignite 5
Ignite 3
Ignite 1
View profile
Jörg_Hoh
Employee

31-03-2017

Hi Daniel,

I am not sure if the relevant classes already implement java.io.closable to work with the try-with statement of Java7.

Avatar

Avatar
Validate 1
Level 3
daniel_henriqu1
Level 3

Likes

15 likes

Total Posts

56 posts

Correct reply

10 solutions
Top badges earned
Validate 1
Boost 5
Boost 3
Boost 10
Boost 1
View profile

Avatar
Validate 1
Level 3
daniel_henriqu1
Level 3

Likes

15 likes

Total Posts

56 posts

Correct reply

10 solutions
Top badges earned
Validate 1
Boost 5
Boost 3
Boost 10
Boost 1
View profile
daniel_henriqu1
Level 3

01-04-2017

To be honest, when I first wrote my answer I had a different solution in mind (I had forgotten about try-withResources).

What are your thoughts about

a "functional" interface representing the code in the limited scope

public interface Action<T> { public T perform(final ResourceResolver resourceResolver) throws PersistenceException; }

a OSGi utility service with a withResourceResolver method:

public <T> T withResourceResolver(final Action<T> action) throws PersistenceException, IllegalStateException { ResourceResolver resourceResolver = null; try { Map<String, Object> props = new HashMap<String, Object>(); props.put(ResourceResolverFactory.SUBSERVICE, SERVICE_NAME); resourceResolver = resourceResolverFactory.getServiceResourceResolver(props); return action.perform(resourceResolver); } catch (LoginException e) { logger.error("Error obtaining the Resource Resolver", e); throw new IllegalStateException(e); } finally { if (resourceResolver != null) { resourceResolver.close(); } } }

invoked by different classes:

String title = this.util.withResourceResolver((ResourceResolver resolver) -> { /* * Do here all the operations involving a resolver and return null or * any kind of object. */ /* For instance: */ Resource resource = resolver.getResource("/content/geometrixx-outdoors.html"); ValueMap props = resource.getValueMap(); return props.get(JcrConstants.JCR_TITLE, String.class); });

?

Regards,

Daniel.

Avatar

Avatar
Coach
Employee
Jörg_Hoh
Employee

Likes

1,134 likes

Total Posts

3,161 posts

Correct reply

1,079 solutions
Top badges earned
Coach
Give back 600
Ignite 5
Ignite 3
Ignite 1
View profile

Avatar
Coach
Employee
Jörg_Hoh
Employee

Likes

1,134 likes

Total Posts

3,161 posts

Correct reply

1,079 solutions
Top badges earned
Coach
Give back 600
Ignite 5
Ignite 3
Ignite 1
View profile
Jörg_Hoh
Employee

01-04-2017

Hi,

I don't care what mechanism you use to make sure, that the session is closed after its usage. I am only interested in the fact, that it's eventually closed 🙂

Jörg

Avatar

Avatar
Validate 1
Level 10
edubey
Level 10

Likes

277 likes

Total Posts

1,502 posts

Correct reply

392 solutions
Top badges earned
Validate 1
Give Back 50
Give Back 5
Give Back 3
Give Back 25
View profile

Avatar
Validate 1
Level 10
edubey
Level 10

Likes

277 likes

Total Posts

1,502 posts

Correct reply

392 solutions
Top badges earned
Validate 1
Give Back 50
Give Back 5
Give Back 3
Give Back 25
View profile
edubey
Level 10

02-04-2017

Nice and Important Stuff,

Thanks for sharing.

Avatar

Avatar
Validate 25
MVP
Ratna_Kumar
MVP

Likes

159 likes

Total Posts

755 posts

Correct reply

134 solutions
Top badges earned
Validate 25
Validate 10
Validate 1
Give Back 50
Give Back 5
View profile

Avatar
Validate 25
MVP
Ratna_Kumar
MVP

Likes

159 likes

Total Posts

755 posts

Correct reply

134 solutions
Top badges earned
Validate 25
Validate 10
Validate 1
Give Back 50
Give Back 5
View profile
Ratna_Kumar
MVP

03-04-2017

Thanks for sharing Jorg,

Its really helpful!!

~ Ratna.