• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Liutauras Vilda
  • Bear Bibeault
  • Junilu Lacar
  • Martin Vashko
Sheriffs:
  • Jeanne Boyarsky
  • Tim Cooke
  • Knute Snortum
Saloon Keepers:
  • Ron McLeod
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Scott Selikoff
  • salvin francis
  • Piet Souris

Peek() Method generating a Major Code Smell in SonarQube

 
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello,

Please, can you provide help about this little stream:

I have a class of and a list of the class:





If I want to validate that we don't have person with age over 200,

 

When I use validateAge method like this:

       

When I use peek() method, SonarQube complains.

My question:Can you help me do this without using peek() method?

Thank you for your response,
Achille
 
author
Posts: 4172
29
jQuery Eclipse IDE Java
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm not 100% sure of your requirements, but you could use map()... something like map(x -> { ....check for age and throw exception... return x;}).

I think a better approach would be just to use filter() though, such as...  stream.parallel().filter(x -> x.getAge()>200).findAny().isPresent()  is true then throw exception.  You could also use stream.parallel().allMatch(x -> x.getAge()<=200) is false then throw exception (or anyMatch on the reverse).

There's probably twenty other ways to do it.  The filter()/allMatch() don't return a sum, although for clarity you can do this as two separate operations since O(2n) = O(n).
 
Marshal
Posts: 66637
251
  • Likes 4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You are validating the age in the wrong place. I presume you mean to have age as an int, not a String. If you are setting age via the Person constructor, that is the place to validate it.You doubtless know already why I didn't take any defensive copies. You would want similar validation in a setAge() method. Otherwise you will have an object in an inconsistent state, which you can't be sure of deleting from memory completely. A static method in a different class violates the single responsibility principle because you are making code outwith the current object take care of that object.

Avoid side‑effects as much as possible when running a Stream. It would disrupt the Stream severely to throw any exceptions (that counts as a side‑effect). You want the Stream code to complete normally
 
Achille Guemkam
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:You are validating the age in the wrong place. I presume you mean to have age as an int, not a String. If you are setting age via the Person constructor, that is the place to validate it.You doubtless know already why I didn't take any defensive copies. You would want similar validation in a setAge() method. Otherwise you will have an object in an inconsistent state, which you can't be sure of deleting from memory completely. A static method in a different class violates the single responsibility principle because you are making code outwith the current object take care of that object.

Avoid side‑effects as much as possible when running a Stream. It would disrupt the Stream severely to throw any exceptions (that counts as a side‑effect). You want the Stream code to complete normally



Sorry for the typo. age is int, not String.
I can't validate in the class because the class in generated by swagger.
There are already some validations  but I have some special cases that I need a particular validation which I have not yet found how to do it in swagger.

What I wrote is just a simplified version of the problem that I have.
I think the tip from Scott is a good direction as I need to refactor my validation to do it in two separate operations.

Thank you for your response Campbell
 
Achille Guemkam
Greenhorn
Posts: 7
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Scott Selikoff wrote:I'm not 100% sure of your requirements, but you could use map()... something like map(x -> { ....check for age and throw exception... return x;}).

I think a better approach would be just to use filter() though, such as...  stream.parallel().filter(x -> x.getAge()>200).findAny().isPresent()  is true then throw exception.  You could also use stream.parallel().allMatch(x -> x.getAge()<=200) is false then throw exception (or anyMatch on the reverse).

There's probably twenty other ways to do it.  The filter()/allMatch() don't return a sum, although for clarity you can do this as two separate operations since O(2n) = O(n).



Hello,

I'm going with your proposition.
I will refactor my validation method to return a boolean, then also refactor my stream in two separate operations.

Thank you Scott!
 
Campbell Ritchie
Marshal
Posts: 66637
251
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Achille Guemkam wrote:. . . Sorry for the typo . . .

Apology accepted

If you copy'n'paste your code, that won't happen again.

I can't validate in the class because the class in generated by swagger. . . .

That sounds very strange.

Thank you for your response Campbell

That's a pleasure

I presume SonarQube won't notice that you haven't validated age, and, as you have heard, you can filter the Stream:-
 
Achille Guemkam
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

If you copy'n'paste your code, that won't happen again.

For that code, I'm not allowed to paste it, sorry!


I presume SonarQube won't notice that you haven't validated age, and, as you have heard, you can filter the Stream:-


This is the solution. I have refactored my code and it is working well!

Really thanks for your help!
 
Campbell Ritchie
Marshal
Posts: 66637
251
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Achille Guemkam wrote:. . . This is the solution. . . .

It is actually a combination of your original code and Scott's suggestion about filtering.

. . . thanks . . .

That's a pleasure
 
Marshal
Posts: 14530
242
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
only one more suggestion: instead of agePredicate, choose a name that describes what that condition represents instead, like youngerThan200 or notAVampire or notMethuselah

That way, the code is more expressive and makes more sense. Avoid names that hint at the implementation. Choose names that express intent.


Without looking at how youngerThan200yrs is defined, you should already be able to form an idea of what it is going to do.

That's not the case with this:

When someone reads that and they're likely to think, "Oh great, now I have to go find out what 'agePredicate' is doing... "
 
It's a beautiful day in this neighborhood - Fred Rogers. Tiny ad:
Sauce Labs - World's Largest Continuous Testing Cloud for Websites and Mobile Apps
https://coderanch.com/t/722574/Sauce-Labs-World-Largest-Continuous
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!