Get objects from list based on object's field

Question

I have a method which takes List<Document> documents as parameters. I created unit test for it:

@Test
 public void getDocumentsWithHighestVersion(){
 List<Document> docs = new ArrayList<>();
 Type type1 = new Type(PDF, ENGLISH);
 docType1.setDocumentTypeId(37);
 Type type2 = new Type(PDF, ENGLISH);
 type2.setDocumentTypeId(31);
 Document doc1 = new Document(1, "name", type1, 7);
 Document doc2 = new Document(2, "name", type2, 7);
 Document doc3 = new Document(5, "name", type1, 4);
 Document doc4 = new Document(6, "name", type2, 4);
 Document doc5 = new Document(8, "name", type1, 1);
 Document doc6 = new Document(9, "name", type2, 1);
 docs.add(doc1);
 docs.add(doc2);
 docs.add(doc3);
 docs.add(doc4);
 docs.add(doc5);
 docs.add(doc6);
 List<Document> docsWithHighestVersion = underTest.getDocsWithHighestVersion(docs);
 assertTrue(docsWithHighestVersion.contains(doc1));
 assertTrue(docsWithHighestVersion.contains(doc2));
 assertTrue(!docsWithHighestVersion.contains(doc3));
 assertTrue(!docsWithHighestVersion.contains(doc4));
 assertTrue(!docsWithHighestVersion.contains(doc5));
 assertTrue(!docsWithHighestVersion.contains(doc6));
 }

so here we have documents with types 31 and 34. The task is to find the document with highest version for every type. So correct answer is doc1 and doc2 because they have version 7.

I created method which seems to work but it looks awful:

protected List<Document> getDocsWithHighestVersion(List<Document> documents) {
 Map<Integer, Document> docsWithHigherVersion = new HashMap<>();
 int numOfDocs = documents.size();
 for (int i = 0; i < numOfDocs - 1; i++) {
 for (int j = i + 1; j < numOfDocs; j++) {
 if (documents.get(i).getType().equals(documents.get(j).getType())) {
 Document docWithHigherVersion = documents.get(i).getVersion() > documents.get(j).getVersion() ? documents.get(i) : documents.get(j);
 if (docsWithHigherVersion.containsKey(docWithHigherVersion.getType().getTypeId())) {
 if(docsWithHigherVersion.get(docWithHigherVersion.getType().getTypeId()).getVersion() < docWithHigherVersion.getVersion()){
 docsWithHigherVersion.put(docWithHigherVersion.getType().getTypeId(), docWithHigherVersion);
 }
 } else {
 docsWithHigherVersion.put(docWithHigherVersion.getType().getTypeId(), docWithHigherVersion);
 }
 }
 }
 }
 return new ArrayList<>(docsWithHigherVersion.values());
 }

How can I make it readable? I am sure there must exists some prettier solution even without streams. I would like to see streams solution also but for above problem I am not allowed to use it.

• Mark Bluemel: I suspect you'll get a lot of guidance on asking a better question. People tend not to like snippets of code, for a start. What does "I am not allowed to use [streams]" mean - is this homework?

Answer 1

I don't quite understand why you have two loops there. One would do just fine.

 protected List<Document> getDocsWithHighestVersion(List<Document> documents) {
 Map<Integer, Document> highestVersionDocumentsByTypeId = new HashMap<>();
 int documentsCount = documents.size();
 for (int i = 0; i < documentsCount; i++) {
 Document document = documents.get(i);
 Integer documentTypeId = document.getType().getTypeId();
 if (highestVersionDocumentsByTypeId.containsKey(documentTypeId)) {
 if (document.getVersion() < highestVersionDocumentsByTypeId.get(documentTypeId).getVersion(){
 continue;
 }
 }
 highestVersionDocumentsByTypeId.put(documentTypeId, document);
 }
 return new ArrayList<>(highestVersionDocumentsByTypeId.values());
 }
````

• Eric Stein: This is a fine observation, but it is not a code review.
• slepic: @Eric Stein And? If I have nothing more to say, or no time to say more, should I just shut up and let OP live in fantasy land of two loops being useful here? Post your own review and don't downvote mine if there is nothing wrong with it except for being succinct.
• Michu93: @EricStein how it's not code review? It makes my code much simpler and readable
• TorbenPutkonen: It's not a code review because it does not explain why a single loop would suffice. The comment does not provide enough clarifying information that would elevate the answer beyond being "just a code snippet." A comment "One loop will do just fine because the version comparison against the current known latest version is already being done in the innermost if-statement." would have been enough to satisfy the requirements.

Answer 2

A bit more readable and easier to implement would be:

• Extract method, that finds the highest version for a single document type.
• Group your documents by type and call the method for every type separately in a (outer) loop to gather your results.
• I would rather use return type for your method Map<Integer, Document>, so you that you can get your highest document by every type, but that depends on your further usage.

Good tip for this kind of methods - if you see 2 nested loops, you usually want to extract the inner loop into separate method. It helps isolating parameters, reduces complexity and (usually) increases readability in combination with good method and parameter naming :-)