• 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
  • Paul Clapham
  • Jeanne Boyarsky
  • Junilu Lacar
  • Henry Wong
Sheriffs:
  • Ron McLeod
  • Devaka Cooray
  • Tim Cooke
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Frits Walraven
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Piet Souris
  • salvin francis
  • fred rosenberger

Java 8/ OO Design

 
Greenhorn
Posts: 25
1
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Can you kindly comment and review this code for design improvements?

The program tries to count occurences and print.

The expected output is to return the number of occurences of the input:


Kindly point out how the solution can be improved upon, or made more readable. It's only a simple problem, but how could I make it more OO?

One example might be, I think I can probably split out the printing part from the method, because the method name is countFrequency() but it also prints. Should I have another class for it?
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Appreciate any feedback on this, please help!
 
Saloon Keeper
Posts: 22126
151
Android Eclipse IDE Tomcat Server Redhat Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Just from a superficial view, I'd say that your example is quite good as it is.

Object-oriented design is designed to make complex tasks simple. But your example is already very compact and easy to read. So I wouldn't complicate it by spinning off new classes just to be more object-oriented.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you. Is it the best way or most efficient way to do it in terms of Java 8 usage?
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Is there anyway to make this return a LinkedHashMap instead of a HashMap so that I can return the output in the same order as the input? I've tried a few things, but none of them seem to work and I am stuck
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've made some progress, but I feel that there's some redundancy in my code, but I am not sure how exactly to simplify it.



Could the last 2 statements be combined - the counting and converting to linked map part? I don't know how to make it both count and also put a default value.
 
Marshal
Posts: 25594
69
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It looks like the answer might be Yes. You used the simplest form of Collectors.groupingBy, but there are others. One of them has a parameter which is suggestively named "mapFactory". The API documentation goes into more detail, including an example of how to specify the type of the Map.

The Collectors class is generics running amuck (five type parameters??!!) so it isn't obvious to me that using a LinkedHashMap is going to do what you want, but I encourage you to try it out.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you. Is there a way to also do the lowercase within the stream function? I convert to lowercase list but then again I return the output with the original list, so could that be simplified as well?
 
Paul Clapham
Marshal
Posts: 25594
69
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Which one? There's several instances of converting to lower case in your code.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok, I was able to get a linked map from groupingBy but I am not sure how to do counting() and also put a default value.



Now this counts, but I need to add the value to the map and put a 0 if the key is not found.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote:Which one? There's several instances of converting to lower case in your code.

Could all of the individual conversions (from input to InputInLowerCase and content to ContentInLowerCase) be avoided and the case ignore comparison be done within the stream and filter functions?
 
Paul Clapham
Marshal
Posts: 25594
69
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There's an "equalsIgnoreCase" method in String, but there's no "containsIgnoreCase" method. So if you could dispense with "contains" and replace it somehow by "equals", you could then use "equalsIgnoreCase" to get rid of the lower-casing.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
How do I form a regex for splitting this type of sentence. I had this , but it gives me a result for space or some blank character.

Sample text:
 
Tim Holloway
Saloon Keeper
Posts: 22126
151
Android Eclipse IDE Tomcat Server Redhat Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Raja Avrv wrote:How do I form a regex for splitting this type of sentence. I had this , but it gives me a result for space or some blank character.



The full-stop (period, dot) character is magic in regexes and matches any character, not just a dot. If you're looking to split apart sentences and comma-separated phrases, a better regex would be "[,\\.]".

Note that I had to escape the escape character (backslash) because that, too is magic when used in Java string literals.
 
Marshal
Posts: 69495
277
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There's a space in that regex, too. I am not sure I like that regex; it will have to get bigger every time you add a punctuation mark. It also risks producing zero‑length Strings whenever separating characters are juxtaposed. I might consider something like this instead, using "\\w+" to match. You can augment that regex to permit included apostrophes and hyphens, etc.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:There's a space in that regex, too. I am not sure I like that regex; it will have to get bigger every time you add a punctuation mark. It also risks producing zero‑length Strings whenever separating characters are juxtaposed. I might consider something like this instead, using "\\w+" to match. You can augment that regex to permit included apostrophes and hyphens, etc.



I currently have this "\\w+" which seems to be working for words including alphabets and numbers only. How should I change it to include special characters within the word, like abc-def and abc'd?
 
Saloon Keeper
Posts: 12027
257
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It's not clear to me whether you want to count "abc-def" as one word or as two words. From your phrasing, I'm assuming that you want it to count as a single word. If that's the case, then what special characters should be exempted besides dashes and apostrophes?

Anyway, the regular expression you're looking for is: (?U:[\W&&[^\p{Pd}']]+)

It means: Split around non-word characters ( \W ), but not dashes ( \p{Pd} ) or apostrophes, and do it all in a Unicode-aware manner ( ?U: ) so that accented and non-ASCII characters like 'é' and 'の' are not counted as non-word characters.

Now, since you are collating words, you really ought to be using a Collator.


Note that this code makes a best effort attempt to be locale aware, but your requirement to exclude dashes and apostrophes breaks full locale awareness because whether these symbols indicate word boundaries is context dependent.
 
Stephan van Hulst
Saloon Keeper
Posts: 12027
257
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you want, you can let the method return a map containing lowercase string keys instead of collation keys, but it adds an additional processing step that the client may not always need.
 
Stephan van Hulst
Saloon Keeper
Posts: 12027
257
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Tim Holloway wrote:The full-stop (period, dot) character is magic in regexes and matches any character, not just a dot. If you're looking to split apart sentences and comma-separated phrases, a better regex would be "[,\\.]".


Character classes use their own set of special characters. The dot is not one of them, so you can write a '.' character inside a character class without escaping it if you mean to use it literally.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you. Can you please review this for unit test of the above method?

 
Stephan van Hulst
Saloon Keeper
Posts: 12027
257
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There's very little I can say about the correctness of your test without knowing what it is testing.

What is subject? Why are you mocking the word counting service?
 
Marshal
Posts: 15638
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Line 10 of the test looks very wrong to me. Especially when you look at line 22.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:There's very little I can say about the correctness of your test without knowing what it is testing.

What is subject? Why are you mocking the word counting service?



subject is the class under test here, the controller. I am mocking the service because the service is the one that's doing the actual work. All the controller does is delegate the work to the service class, by passing the input (list of strings) it receives and another list of strings (to search in).
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Line 10 of the test looks very wrong to me. Especially when you look at line 22.



The service class actually does all the computation, so I thought I could just test that the controller calls the service (using verify) here and that I can see the result of that service call come out of the controller (through expect). Is that expect maybe not necessary?

 
Junilu Lacar
Marshal
Posts: 15638
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Raja Avrv wrote:
The service class actually does all the computation, so I thought I could just test that the controller calls the service (using verify) here and that I can see the result of that service call come out of the controller (through expect). Is that expect maybe not necessary?


1. Your test method name does not give that impression
2. The test code does not clearly communicate its intent
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

Raja Avrv wrote:
The service class actually does all the computation, so I thought I could just test that the controller calls the service (using verify) here and that I can see the result of that service call come out of the controller (through expect). Is that expect maybe not necessary?


1. Your test method name does not give that impression
2. The test code does not clearly communicate its intent



Please tell me what changes I can make to make it convey intent better.
 
Junilu Lacar
Marshal
Posts: 15638
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You can start by showing us the code in this service class that you want to test.
 
Stephan van Hulst
Saloon Keeper
Posts: 12027
257
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The most important change is a descriptive name for your test.

Why do your service and controller return a Count object? Does it do anything other than act as a wrapper for a Map?

Does your controller do anything more than just forward the request to the counting service? Like, does it transform the result to a different type, or throw any exceptions in certain circumstances? If not, then I don't see the point of unit testing the controller at all.
 
Raja Avrv
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok, I'll post most of my code here (did a little bit of refactoring) to provide a complete picture.

Controller:


Service:


ControllerTest:


ServiceTest:
 
Junilu Lacar
Marshal
Posts: 15638
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
See this tutorial for how you want to test a Controller class: https://spring.io/guides/gs/testing-web/

Consider: are your current controller tests redundant with the service tests? (Hint: they should test different things)
 
Junilu Lacar
Marshal
Posts: 15638
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
On your first service test, its name says "it should return highest frequency words"

However, the test code is doing more than that:
1. Uses a count of 2
2. Uses a data set with only one word that is repeated ("four") and multiple words that appear only once
3. Yet, the expected result contains "the", which appears only once. It's not clear why "the" is expected in the result rather than any of the other words that appear only once in the data being searched.

The test name should be a concise yet complete summary of what the test code is doing. This is not the case here. This creates cognitive dissonance and makes your test difficult to comprehend.
 
There is no "i" in denial. Tiny ad:
Devious Experiments for a Truly Passive Greenhouse!
https://www.kickstarter.com/projects/paulwheaton/greenhouse-1
    Bookmark Topic Watch Topic
  • New Topic