Converting between backend object and object suitable for JSON

Question

I wrote this method that does the job and passes the assertion tests for a large java backend project but I'm not all happy with the way it reads, perhaps you can tell me how to refactor and/or improve this method?

public Set<ProductOfferingJsonDto> getJsonProductOfferings() {
 Set<ProductOfferingJsonDto> set = new HashSet<ProductOfferingJsonDto>();
 ProductOfferingJsonDto pjson = null;
 Iterator iter = productOfferings.iterator();
 while(iter.hasNext()){
 ProductOfferingDto pdto = (ProductOfferingDto) iter.next();
 if(pdto instanceof BundledProductOfferingDto) {
 Set<ProductOfferingDto> tmp = ((BundledProductOfferingDto) pdto).getProductOfferings();
 Iterator piter = tmp.iterator();
 Set<ProductOfferingJsonDto> tmpset = new HashSet<ProductOfferingJsonDto>();
 while(piter.hasNext()){
 SimpleProductOfferingDto piterdto = (SimpleProductOfferingDto) piter.next();
 tmpset.add(new SimpleProductOfferingJsonDto(piterdto.getId(), piterdto.getName(), piterdto.getDescription(), piterdto.getStatus(),
 piterdto.getBrand(), piterdto.getValidFor(),
 piterdto.getProductSpecification(),
 piterdto.getDescriptorId(), piterdto.getTags(), piterdto.getProductOfferingPrice()));
 }
 BundledProductOfferingDto bpod = (BundledProductOfferingDto) pdto;
 pjson = new BundledProductOfferingJsonDto(bpod.getId(), bpod.getName(), bpod.getDescription(), bpod.getStatus(),
 bpod.getBrand(), bpod.getValidFor(),
 tmpset,
 bpod.getDescriptorId(), bpod.getTags(), bpod.getProductOfferingPrice());
 }
 if(pjson != null) {
 set.add((ProductOfferingJsonDto) pjson);
 }
 }
 return set;
}

• ZijingWu: What's the problem of this code? Why you thing it is hard to reader and need refactor? You need a goal for reflector code, so other people can help you.
• Niklas Rosencrantz: @ZijingWu I think the code is difficult to understand what it does. I wanted to make it with smaller units that are more clear what they do. I'm converting between backend object and object suitable for JSON. The goal is to add a flad for leaf at leaf nodes and generated unique IDs over classes while IDs now are unique only within a class, therefore I'm making specific objects for the json requirement.

Answer 1

I would move the construction of the JSON objects into factory methods like this:

public class BundledProductOfferingJsonDto
{
 ....
 public static BundledProductOfferingJsonDto fromDto(BundledProductOfferingDto dto, Set<ProductOfferingJsonDto> productOfferings)
 {
 return new BundledProductOfferingJsonDto(dto.getId(), dto.getName(), dto.getDescription(), dto.getStatus(),
 dto.getBrand(), dto.getValidFor(),
 productOfferings,
 dto.getDescriptorId(), dto.getTags(), dto.getProductOfferingPrice());
 }
}

Similar for SimpleProductOfferingJsonDto

Also, is there a reason why your are using the while loops with the iterators instead of for example for (ProductOfferingDto piter : tmp)?

The method could then read something like this:

public Set<ProductOfferingJsonDto> getJsonProductOfferings() {
 Set<ProductOfferingJsonDto> set = new HashSet<ProductOfferingJsonDto>();
 ProductOfferingJsonDto pjson = null;
 for (ProductOfferingDto pdto : productOfferings) {
 if (pdto instanceof BundledProductOfferingDto) {
 Set<ProductOfferingDto> tmp = ((BundledProductOfferingDto) pdto).getProductOfferings();
 Set<ProductOfferingJsonDto> tmpset = new HashSet<ProductOfferingJsonDto>();
 for (ProductOfferingDto piter : tmp) {
 SimpleProductOfferingDto piterdto = (SimpleProductOfferingDto)piter;
 tmpset.add(SimpleProductOfferingJsonDto.fromDto(piterdto));
 }
 BundledProductOfferingDto bpod = (BundledProductOfferingDto) pdto;
 BundledProductOfferingJsonDto pjson = BundledProductOfferingJsonDto.fromDto(bpod, tmpset));
 set.add(pjson);
 }
 }
 return set;
}

• Max: I'm giving you an upvote. Even though we got the same answer (and mine slightly faster :P) I think your snippet reads better.
• ChrisWue: @user1021726: hehe, funny, same idea :)
• Florian Salihovic: You can eliminate the last if condition by putting set.add(ProductOfferingJsonDto) pjson) into the first condition, when the JSON instance is created. This would eliminate the need for json as well and so this method could be reduced to 15 lines.

Answer 2

You could add a factory method for SimpleProductOfferingJsonDto and BundledProductOfferingJsonDto which only takes a piterdto and a bpod object.

It could look something like:

public Set<ProductOfferingJsonDto> getJsonProductOfferings() {
Set<ProductOfferingJsonDto> set = new HashSet<ProductOfferingJsonDto>();
ProductOfferingJsonDto pjson = null;
Iterator iter = productOfferings.iterator();
while(iter.hasNext()){
 ProductOfferingDto pdto = (ProductOfferingDto) iter.next();
 if(pdto instanceof BundledProductOfferingDto) {
 Set<ProductOfferingDto> tmp = ((BundledProductOfferingDto) pdto).getProductOfferings();
 Iterator piter = tmp.iterator();
 Set<ProductOfferingJsonDto> tmpset = new HashSet<ProductOfferingJsonDto>();
 while(piter.hasNext()){
 SimpleProductOfferingDto piterdto = (SimpleProductOfferingDto) piter.next();
 tmpset.add(new NewJSONSPOD(piterdto));
}
 BundledProductOfferingDto bpod = (BundledProductOfferingDto) pdto;
 pjson = new NewJSONBPOD(bpod);
 if(pjson != null) {
 set.add((ProductOfferingJsonDto) pjson);
 }
}
return set;
public static BundledProductOfferingJsonDto NewJSONBPOD(BundledProductOfferingDto bpod) {
 return new BundledProductOfferingJsonDto(bpod.getId(), bpod.getName(), bpod.getDescription(), bpod.getStatus(),
 bpod.getBrand(), bpod.getValidFor(),
 tmpset,
 bpod.getDescriptorId(), bpod.getTags(), bpod.getProductOfferingPrice());
}
public static SimpleProductOfferingJsonDto NewJSONSPOD(SimpleProductOfferingDto piterdto) {
 return new SimpleProductOfferingJsonDto(piterdto.getId(), piterdto.getName(), piterdto.getDescription(), piterdto.getStatus(),
 piterdto.getBrand(), piterdto.getValidFor(),
 piterdto.getProductSpecification(),
 piterdto.getDescriptorId(), piterdto.getTags(), piterdto.getProductOfferingPrice()));
}

You might want to change some names etc, but you get the gist of it