Getting wrong output from the service

Question

I am working on logic, in which I have to consume, List of PersonDto, and apply rules based on the rule value(private String rule;) present in the PersonDto.

I have an interface called Rules, which will have multiple implementations. But for this question, I have added only 1 implementation called Rule1.

I also have a RuleMapper class which is responsible for providing beans on demand.

Problem: Based on the rule value present in the PersonDto, I am applying the rules. Inside Rule1 class I have a logic to modify the id field. I am setting new id which I am getting as a parameter.
And finally, I am storing the result into ArrayList.
But in the ArrayList, the value of all the PersonDto is coming as the last id value that I am passing while applying the rule.

For Example:

 List<String> ids = Arrays.asList("10001", "100002");
    List<PersonDto> result = new ArrayList<PersonDto>();

    persons.stream().forEach(person -> {
        ids.stream().forEach(id -> {
            System.out.println(ruleMapper.getRule(person.getRule()).applyRule(person, id));

            result.add(ruleMapper.getRule(person.getRule()).applyRule(person, id));

        });
    });

As you can see in the above code snippet, there are two ids 10001 & 10002, but when I am storing and then printing the result, then I am getting PersonDto with the id value as 10002 in all the elements. But when I am doing System.out.println(), inside the loop, then I am seeing the correct result.

For more information or to reproduce the problem, please refer to the complete code below:

PersonDto

package com.example.dto;

public class PersonDto {
protected int id;
protected String name;
private String rule;

public int getId() {
    return id;
}

public void setId(int id) {
    this.id = id;
}

public String getName() {
    return name;
}

public void setName(String name) {
    this.name = name;
}



public String getRule() {
    return rule;
}

public void setRule(String rule) {
    this.rule = rule;
}

@Override
public String toString() {
    return "PersonDto [id=" + id + ", name=" + name + ", rule=" + rule + "]";
}

}

Rules interface

package com.example.service;

import com.example.dto.PersonDto;

public interface Rules {

public PersonDto applyRule(PersonDto input, String newId);

}

Rules 1 implementation

package com.example.service;

import org.springframework.stereotype.Service;

import com.example.dto.PersonDto;

@Service
public class Rule1 implements Rules {

@Override
public PersonDto applyRule(PersonDto input, String newIdt) {
    input.setId(Integer.parseInt(newIdt));
    return input;
}

}

RuleMapper

package com.example.service;

import java.util.Map;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;

@Service

public class RuleMapper {

@Autowired
private Map<String, Rules> beans;

public Rules getRule(String ruleName) {
    return beans.get(ruleName);
}

public Map<String, Rules> gelAllBeans() {
    return beans;
  }
}

Controller

package com.example.demo;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Random;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;

import com.example.dto.PersonDto;
import com.example.service.RuleMapper;

@RestController
public class StudentContoller {

@Autowired
private RuleMapper ruleMapper;

@GetMapping(value = "/test")
public void saveStudent() throws Exception {
    List<String> orders = Arrays.asList("order 1", "order 2");

    List<PersonDto> persons = new ArrayList<PersonDto>();

    for (int i = 0; i < 10; i++) {
        PersonDto per = new PersonDto();
        per.setId(i);
        per.setName("John Doe_ ".concat(String.valueOf(i)));
        per.setRule("rule" + getRandomRule());
        persons.add(per);
    }


    List<String> ids = Arrays.asList("10001", "100002");
    List<PersonDto> result = new ArrayList<PersonDto>();

    persons.stream().forEach(person -> {
        ids.stream().forEach(id -> {
            System.out.println(ruleMapper.getRule(person.getRule()).applyRule(person, id));

            result.add(ruleMapper.getRule(person.getRule()).applyRule(person, id));

        });
    });


    for (PersonDto person : result) {
        System.out.println(person);
    }

}

private int getRandomRule() {
    Random r = new Random();
    int low = 1;
    int high = 2;
    int result = r.nextInt(high - low) + low;

    return result;
 }

}

Expected output

PersonDto [id=10001, name=John Doe_ 0, rule=rule1]
PersonDto [id=100002, name=John Doe_ 0, rule=rule1]
PersonDto [id=10001, name=John Doe_ 1, rule=rule1]
PersonDto [id=100002, name=John Doe_ 1, rule=rule1]
PersonDto [id=10001, name=John Doe_ 2, rule=rule1]
PersonDto [id=100002, name=John Doe_ 2, rule=rule1]
PersonDto [id=10001, name=John Doe_ 3, rule=rule1]
PersonDto [id=100002, name=John Doe_ 3, rule=rule1]
PersonDto [id=10001, name=John Doe_ 4, rule=rule1]
PersonDto [id=100002, name=John Doe_ 4, rule=rule1]
PersonDto [id=10001, name=John Doe_ 5, rule=rule1]
PersonDto [id=100002, name=John Doe_ 5, rule=rule1]
PersonDto [id=10001, name=John Doe_ 6, rule=rule1]
PersonDto [id=100002, name=John Doe_ 6, rule=rule1]
PersonDto [id=10001, name=John Doe_ 7, rule=rule1]
PersonDto [id=100002, name=John Doe_ 7, rule=rule1]
PersonDto [id=10001, name=John Doe_ 8, rule=rule1]
PersonDto [id=100002, name=John Doe_ 8, rule=rule1]
PersonDto [id=10001, name=John Doe_ 9, rule=rule1]
PersonDto [id=100002, name=John Doe_ 9, rule=rule1]

Actual Output

PersonDto [id=100002, name=John Doe_ 0, rule=rule1]
PersonDto [id=100002, name=John Doe_ 0, rule=rule1]
PersonDto [id=100002, name=John Doe_ 1, rule=rule1]
PersonDto [id=100002, name=John Doe_ 1, rule=rule1]
PersonDto [id=100002, name=John Doe_ 2, rule=rule1]
PersonDto [id=100002, name=John Doe_ 2, rule=rule1]
PersonDto [id=100002, name=John Doe_ 3, rule=rule1]
PersonDto [id=100002, name=John Doe_ 3, rule=rule1]
PersonDto [id=100002, name=John Doe_ 4, rule=rule1]
PersonDto [id=100002, name=John Doe_ 4, rule=rule1]
PersonDto [id=100002, name=John Doe_ 5, rule=rule1]
PersonDto [id=100002, name=John Doe_ 5, rule=rule1]
PersonDto [id=100002, name=John Doe_ 6, rule=rule1]
PersonDto [id=100002, name=John Doe_ 6, rule=rule1]
PersonDto [id=100002, name=John Doe_ 7, rule=rule1]
PersonDto [id=100002, name=John Doe_ 7, rule=rule1]
PersonDto [id=100002, name=John Doe_ 8, rule=rule1]
PersonDto [id=100002, name=John Doe_ 8, rule=rule1]
PersonDto [id=100002, name=John Doe_ 9, rule=rule1]
PersonDto [id=100002, name=John Doe_ 9, rule=rule1]

JB Nizet · Answer 1 · 2019-12-29T13:09:25.253

0

persons.stream().forEach(person -> {
    ids.stream().forEach(id -> {
        result.add(ruleMapper.getRule(person.getRule()).applyRule(person, id));
    });
});

This loops through all persons. And for each person, it loops through all the IDs. For each ID, you apply the rule on the person. But since the rule consists in modifying the ID of the person, the last ID is the one stored in the person.

If you rewrite is as for loops, it's equivalent to

for (Person person: persons) {
    for (String id: ids) {
        person.setId(id);
    }
}

I'm not sure what the code is supposed to do instead. Maybe the first ID should be set on the first person, the second ID to the second, etc. If that's the case, then loop through the indices of one of the list, and get the person and the ID for each index. (And pray for the two lists to have the same size):

for (int i = 0; i < persons.size(); i++) {
    persons.get(i).setIf(ids.get(i));
}

edited Dec 29 '19 at 13:09

answered Dec 29 '19 at 13:06

JB Nizet

678,734
91
1,224
1,255

Thanks for the answer, but the requirement is like that only. Let's say there are 10 persons and there are 2 ids, then the output should be 20 records and each person should have those 2 ids. Please refer to the above **Expected** && **Actual** ouputs. – mayank bisht Dec 29 '19 at 13:09
That makes no sense. if you want a person to have two IDs, then Person shouldn't have a field `int id`. It should have a field `List ids`. You can't store two IDs in a single int variable. Or you need to create a **copy** of each person for each ID. – JB Nizet Dec 29 '19 at 13:10
No, not like that. Let's say there are two person[p1,p2] & there are 2 ids[1001,1002]. Now the output should be [p1 with id:1001, p1 with id:1002, p2 with id:1001, p2 with id:1002 ]. – mayank bisht Dec 29 '19 at 13:12
Yes, I understand that. Which means you need to make **copies** of each person. A unique Person objects can't have two different IDs at the same time. So you need copies. – JB Nizet Dec 29 '19 at 13:14
yes, I am applying the rule and then I am storing the result into the list. If I do System.out.println(), then the output is coming correct, but when I am storing the result in the list, then the person id is getting replaced with the last **id** value in all the list. – mayank bisht Dec 29 '19 at 13:26
Yes, that's expected, since you're not creating a copy of the person. Java passes **references** to objects. A copy of the person is not created when you store it in a list. You just store a **reference** to an object. So you're storing multiple references to the same objects. Read https://stackoverflow.com/questions/40480/is-java-pass-by-reference-or-pass-by-value – JB Nizet Dec 29 '19 at 13:30
No, not every value inside the Person object is changing, only id is changing. – mayank bisht Dec 29 '19 at 15:07
Also, if I hardcode id field inside the Rule1, then I am getting the hardcoded value.Why it's not changing – mayank bisht Dec 29 '19 at 15:07
So what? Create a copy of the person, and keep everything as is, except the ID. – JB Nizet Dec 29 '19 at 15:07
Yup, I did that as well. I created new Person object inside the Rule1, but still getting same output. – mayank bisht Dec 29 '19 at 15:10
Even, I tried to apply rules one by one instead of a loop, untill 2 entries I am getting correct result, but the moment I am inserting the third element, then again output is messing up. – mayank bisht Dec 29 '19 at 15:14
If you want help, edit your question and add a section showing the new code of your rule. It should create a neww PersonDTO, copy all the fields from the original person to this copy, set the ID f the copy using the rule, and return the copy. – JB Nizet Dec 29 '19 at 15:17
Yup, you are right. Earlier I was creating an object and was assigning old values with **=** sign, It turns out again I will be referring to the same address. Now I created a new object and did something like this **Person p =new Person(); p.setRule(input.getRule());, and now the code is working perfectly fine. – mayank bisht Dec 29 '19 at 15:30
But, practically, **input** will have 100's of field, I cannot do property setting for all the 100 fields. – mayank bisht Dec 29 '19 at 15:31
Is there any other way to copy all the values, to the newly created object? – mayank bisht Dec 29 '19 at 15:32
Yes, you can. If you were able to declare and initialize these 100 fields, why wouldn't you be able to copy them? But maybe that should make you think about the idea of having 100 fields in a single class? How about grouping those fields into several other classes? A User doesn't have a street, a city and a country. It has an address. – JB Nizet Dec 29 '19 at 15:33
Okay, actually I am planning to expose only **public PersonDto applyRule(PersonDto input, String newIdt)** method to other developers, so that they just have to implement that function. – mayank bisht Dec 29 '19 at 15:36
They will receive the Object, and they will manipulate it and return it. That's it. – mayank bisht Dec 29 '19 at 15:38
I don't want them to again copy the values. – mayank bisht Dec 29 '19 at 15:38
Then do it yourself, and pass the copy to the rule. – JB Nizet Dec 29 '19 at 15:47

Michael Mesfin · Answer 2 · 2019-12-29T18:14:14.493

0

@Override
public PersonDto applyRule(PersonDto input, String newIdt) {
            PersonDto p =  new PersonDto();
            //copy the value of input to p
            p.setId(Integer.parseInt(newIdt));
            return p;
}

instead of returning a list you can return a Map<Integer,List<PersonDto>>

Map<Integer,List<PersonDto>> dtoMap =  ids.stream()
              .flatMap(i -> persons.stream().map(p -> Rule1.applyRule(p, i)))
                .collect(Collectors.groupingBy(arr -> (Integer)arr[0]
                        ,Collectors.mapping(arr-> (PersonDto)arr[1],Collectors.toList())));

@Override
public PersonDto applyRule(PersonDto input, String newIdt) {
            Object [] dto = {Integer.parseInt(newIdt),input};
            return dto;
}

then when you save to the db

dtoMap.forEach((k,v) -> {
            v.forEach(p -> {
                p.setId(k);
                //save to db
            } );
        });

edited Dec 29 '19 at 18:14

answered Dec 29 '19 at 15:20

Michael Mesfin

546
4
11

Thanks for the answer, this is working fine, but practically **PersonDto** will have 100's of fields and doing setValues for all the fields will be time-consuming. Is there any other way? – mayank bisht Dec 29 '19 at 15:34
cloning might help https://stackoverflow.com/questions/869033/how-do-i-copy-an-object-in-java – Michael Mesfin Dec 29 '19 at 15:48
or maybe you can model PersonDto to contain list of ids – Michael Mesfin Dec 29 '19 at 15:50
**or maybe you can model PersonDto to contain list of ids**, can you please give an example? – mayank bisht Dec 29 '19 at 16:47
Also, for now we are talking about only the **String** type, but it will be an Object. – mayank bisht Dec 29 '19 at 16:51
if you want two id's to be associated with the same object then naturally it's one to many relation so ```protected List ids``` – Michael Mesfin Dec 29 '19 at 17:32
what are you trying to do with the result? – Michael Mesfin Dec 29 '19 at 17:46
After that method returns the result, then again the output will be passed to another method, which will again process it & persist it. – mayank bisht Dec 29 '19 at 17:48
how can you save two PersonDto's with the same id? – Michael Mesfin Dec 29 '19 at 17:59
That's okay, we can save it. It's a business requirement. – mayank bisht Dec 29 '19 at 18:01
But still I have to perform a deep copy, in order to retain the values. – mayank bisht Jan 01 '20 at 12:50
since you will be iterating over the ids, each person will have that id before the update operation – Michael Mesfin Jan 01 '20 at 13:07
I will prefer to go with the list, instead of the map, because of the contract of the subsequent method contract. The next method is expecting the list parameter. I can again convert the map into the list, before calling the subsequent method, but that would unnecessarily add an additional step. So, I will stick to deep copy. Thanks for your support. – mayank bisht Jan 04 '20 at 03:27

Getting wrong output from the service

2 Answers2