Skip to content

Conversation

@fokevolt
Copy link
Owner

Created tree tests.

@fokevolt fokevolt requested a review from roshakorost May 24, 2018 13:23


List<User> nodes = tree.getElements();
for (int i = 0; i < arr.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't tested that the order is actually correct.
Should be something like these:
assertEquals(Arrays.asList(andrii, rostik, //place all in correct order), nodes)
in order to test that order is correct.

@fokevolt
Copy link
Owner Author

Added fix to tree test and added words counter with tests.


public class Category {
private Categories category;
private List<Product> products = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be final

import java.util.List;

public class Category {
private Categories category;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be final.

return category;
}

public List<Product> getProducts() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not return list outside the class it make encapsulation weaker and sometimes causes bugs that are hard to investigate if some class outside adds something to the list.
Rather make copy of the list before returning it new ArrayList(products) or return immutable view of the list Collection.unidentifiableList(products)


public class Product {

private List<Category> categoryList;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be final

private int count;
private int prise;

public List<Category> getCategories() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above comment about returning list directly.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making list final solves the problem of returning it directly. Okay

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it will not.
final List<Category> categoryList = new ArrayList<>();
final doesn't allow to reassign link categoryList but it allows to add elements to list. e.g.
So caller of your method can do the following, that may cause bugs.

List<Category> categoryList product.getCategories()
categotyList.clear()

So I recommend to return copies or immutable copies.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thank you!


private String[] convertToArray(String string){
string = string.trim();
String[] wordArray = string.split(" ");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can inline simple exptestions:
return string.trim().splt(" ")
string is terrible name) use input or something like that.

return false;
} else {
for (String word : convertToArray(string)) {
if (words.containsKey(word)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be simplified:
int count = words.getOrDefault(word, 0)
words.put(word, count + 1)

}

public Map<String, Integer> getFrequency(){
return words;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not return Map directly make a copy new HashMap(words) or Collections.unmodifiableMap(words)

return categoriesOfProduct;
}

public void addProduct (Categories[] category, String name, int count, int price){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can improve your method signature match by using varargs: https://www.geeksforgeeks.org/variable-arguments-varargs-in-java/
public void addProduct (String name, int count, int price, Categories... category)
so you can call it in the following way:
addProduct("sdf", 5, Categories.Sweet,Categories.Chocolate)

}

@Test
public void frequency(){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will recommend to simplify test.
You can call several times count
e.g. count('Hello world"), count("Hello Vetal"), count("a")

Map<String, Integer> expected = new HashMap<>()
expected.put("hello", 2)
expected.put("world", 1)
...
assertEquals(expected, counter.getFrequency)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants