10

I went little fancy and wrote Selenium page-object with Java 8 streaming as mentioned in below code and got a review comment that my code is breaking Law of Demeter, since I am doing lot of operations in a single line. I was suggested to break the code to first stream to collect to list and run another stream operation to do match( in short break it down into multiple streams as needed). I was not convinced as Stream is introduced for handling data processing and if we break it down into multiple streams, there is no point using stream. Previously I have worked for a cyber security project, where millions of records were processed with streaming with multiple logic operations to sort the data.

Please share your thoughts, I have changed it as the reviewer suggested but he couldn't explain why and I want to learn more about streams and right way of utilizing this powerful addition to java 8.

Here is the sample code:

listOfWebElements.stream().filter(element -> element.getText().contains(name)).findFirst().ifPresent(match -> match.click());

is line I am referring to in this question, providing the method so that it makes more sense. 

@FindBy(css = "#someTable td.some-name li a") List<WebElement> listOfWebElements;

public SomeClass doSomething(String name) {

    wait.until(visibilityOfAllElements(listOfWebElements));
    listOfWebElements.stream().filter(element -> element.getText().contains(name)).findFirst()
            .ifPresent(match -> match.click());
    return new SomeClass(driver);

}
Stefan Zobel
  • 3,182
  • 7
  • 28
  • 38
Mrignainee
  • 103
  • 6

2 Answers2

14

Java 8 streams are an example of a fluent interface, and are intended to allow writing in a functional programming style. There is a lot of disagreement over what breaks the LoD and whether it even matters, but all that aside, the example in the documentation for Stream shows that you are using it as intended by the Java language designers.

If that isn't enough for your reviewer, note that the purpose of the Law of Demeter (aka the principle of least knowledge) is to keep programs loosely-coupled by minimizing the number of classes that a class directly communicates with. When A has a B and B has a C, and you want A to make C do something, you should have it tell B to get it done and let B worry about the details of how and when C is used.

In this case, every intermediate method on Stream is returning another instance of Stream, so you are still only coupled to the Stream class. I would not consider this a Law of Demeter violation.

I would also say that any class in a language should be considered coupled to the language's standard library. Therefore, any standard library objects should be exempt from the Law of Demeter, since you can't uncouple from them anyway. Otherwise, you wouldn't even be able to call get on a List that was returned by an object, or deal with a Set of Map.Entry from Map.entrySet(). This would also cover Stream, of course.

I was suggested to break the code to first stream to collect to list and run another stream operation to do match( in short break it down into multiple streams as needed).

Storing intermediate objects in local variables does not fix a Law of Demeter violation, you would still be accessing objects returned by other objects. It sounds like your reviewer is just blindly dot-counting.

Sean Van Gorder
  • 3,393
  • 26
  • 26
  • Thanks for explaining, it makes so much sense now :) – Mrignainee Aug 03 '17 at 20:07
  • Also I was suggested that single line streaming code is not readable so for readability purpose it was suggested to break it down. – Mrignainee Aug 03 '17 at 20:14
  • I would suggest linebreaks before each `.method`, like in the documentation example. It takes some time to get used to the functional programming style, but once you understand it it's much clearer than for loops. I like to think of it as machines on an assembly line, with the stream elements on a conveyor belt traveling through and being changed by `map`, discarded by `filter`, etc. – Sean Van Gorder Aug 03 '17 at 20:20
  • Agree, linebreaks at each .method solves the readability issue. – Mrignainee Aug 03 '17 at 20:39
-1

Here is a detailed explanation of the Law of Demeter. There is some gray area of course, but it is actually quite well defined.

As the answer from Sean Van Gorder pointed out, working with Streams in itself does not constitute a violation of the LoD, neither does chaining method calls.

What is however most likely a violation is this part:

element.getText().contains(name);

According to LoD, you can not access/call a method on anything that you don't have direct access to. I assume, that getText() returns some internal state of the element. That is not something you have direct access to, therefore the contains(name) method call is illegal.

Robert Bräutigam
  • 7,514
  • 1
  • 20
  • 38
  • What would your alternative be? [`WebElement`](https://seleniumhq.github.io/selenium/docs/api/java/org/openqa/selenium/WebElement.html) is from the Selenium library, and it doesn't have a `textContains(String)` method that he could call instead. Libraries that return strings generally expect you to call any standard `String` methods yourself, if needed. This is exactly the kind of thing I was talking about with standard library objects. – Sean Van Gorder Aug 11 '17 at 17:39
  • Even when we don't use stream, if the logic demands some condition check, for e.g if the text of WebElement contains some string return true, we have to use element.getText().contains(name); so I am not sure how LoD comes in picture as we use or not use stream, we have write this method anyway? And isn't stream filter is used for that specific purpose? – Mrignainee Aug 15 '17 at 21:12