patternjavaMinor
Merge two linked list
Viewed 0 times
listtwolinkedmerge
Problem
Given two linkedlist, merge them. Looking for code review, optimizations and best practices.
The code is influenced by feedback here.
Changes incorporated were:
```
public class MergeLinkedList {
private Node first;
private Node last;
private int size;
MergeLinkedList(List items) {
for (Integer i : items) {
add(i);
}
}
public void add (int val) {
final Node newNode = new Node(val);
if (first == null) {
first = last = newNode;
} else {
last.next = newNode;
last = last.next;
}
size++;
}
private static class Node {
private int item;
private Node next;
Node(int element) {
this.item = element;
}
}
private Node mergeLinkedListRecursive(Node node1, Node node2) {
if (node1 == null) {
return node2;
}
if (node2 == null) {
return node1;
}
if (node1.item ());
l1.mergeLinkedListRecursion(l2);
MergeLinkedList expected1 = new MergeLinkedList(Arrays.asList(1, 2, 3));
assertTrue(expected1.equals(l1));
}
@Test
public void testDifferentSizeList() {
MergeLinkedList l1 = new MergeLinkedList(Arrays.asList(1, 3, 5));
MergeLinkedList l2 = new MergeLinkedList(Arrays.asList(2, 4));
l1.mergeLinkedList(l2);
MergeLinkedList expected1Recurse = new MergeLinkedList(Arrays.asList(1, 2, 3, 4, 5));
assertTrue(expected1Recurse.equals(l1));
}
@Test
public void testSameSizeList() {
MergeLinkedList l1 = new MergeLinkedList(Arrays.asList(1, 2, 3));
MergeLinkedList l2 = new MergeLinkedList(Arrays.asList(5, 6, 7));
l1.mergeLinkedList(l2);
The code is influenced by feedback here.
Changes incorporated were:
- Linkedlist is no-longer write-only
- Constructor is not the only way to add new elements
- HashCode and Equals are more meaningful
- Merge (non-recursive) function is now eliminating code-duplication
```
public class MergeLinkedList {
private Node first;
private Node last;
private int size;
MergeLinkedList(List items) {
for (Integer i : items) {
add(i);
}
}
public void add (int val) {
final Node newNode = new Node(val);
if (first == null) {
first = last = newNode;
} else {
last.next = newNode;
last = last.next;
}
size++;
}
private static class Node {
private int item;
private Node next;
Node(int element) {
this.item = element;
}
}
private Node mergeLinkedListRecursive(Node node1, Node node2) {
if (node1 == null) {
return node2;
}
if (node2 == null) {
return node1;
}
if (node1.item ());
l1.mergeLinkedListRecursion(l2);
MergeLinkedList expected1 = new MergeLinkedList(Arrays.asList(1, 2, 3));
assertTrue(expected1.equals(l1));
}
@Test
public void testDifferentSizeList() {
MergeLinkedList l1 = new MergeLinkedList(Arrays.asList(1, 3, 5));
MergeLinkedList l2 = new MergeLinkedList(Arrays.asList(2, 4));
l1.mergeLinkedList(l2);
MergeLinkedList expected1Recurse = new MergeLinkedList(Arrays.asList(1, 2, 3, 4, 5));
assertTrue(expected1Recurse.equals(l1));
}
@Test
public void testSameSizeList() {
MergeLinkedList l1 = new MergeLinkedList(Arrays.asList(1, 2, 3));
MergeLinkedList l2 = new MergeLinkedList(Arrays.asList(5, 6, 7));
l1.mergeLinkedList(l2);
Solution
Wow! You've obviously put a lot of work into this!
Advice / Optimizations
This is how I would write it. (This is likely a school assignment so I'm just supplying a skeleton.)
Stupid Picky Stuff
Advice / Optimizations
- Unless it's an explicit requirement of the assignment, there's no reason you can't use
java.util.LinkedList.
- I think it's cleaner to have the
mergeLinkedListsmethod live in a different class from the linked list. In other words, the linked list is a completely reusable container for data, and the customizations live somewhere else.
- If you want to get fancy you can use generics so that this class can operate on any
Object, not justInteger. (This should be considered "extra credit," not a critique of your program.)
This is how I would write it. (This is likely a school assignment so I'm just supplying a skeleton.)
public class Merge {
private Merge() { } // no public constructor
public static List merge(List list1, List list2) {
List returnVal = new LinkedList<>();
if (list1 == null && list2 == null) {
// both lists null. What to return? Or throw exception.
}
if (list1 == null) {
// What to return? Or throw exception.
}
if (list2 == null) {
// What to return? Or throw exception.
}
// do merging
while (!list1.isEmpty() && !list2.isEmpty()) {
// We know that both lists must have at least one element.
// 1. Figure out which list has the smaller element.
// 2. Remove that item from its list.
// 3. Add that item to the end of returnVal
}
// Now one or both lists are empty.
// Those items have to be added to "returnVal" as well.
return returnVal;
}
public static List mergeRecursive(List list1, List list2) {
// do stuff
}
}Stupid Picky Stuff
- You don't need to have a name like
mergeLinkedListRecursionif the parameter is already of typeLinkedList. You can just call itmergeRecursion. Anybody reading it will understand that it operates on linked lists.
- I don't get why you have functions with names of "recursive", "recursion", and "recurse". It seems to me it would be cleaner to use one verb form for all your functions.
Code Snippets
public class Merge {
private Merge() { } // no public constructor
public static List<Integer> merge(List<Integer> list1, List<Integer> list2) {
List<Integer> returnVal = new LinkedList<>();
if (list1 == null && list2 == null) {
// both lists null. What to return? Or throw exception.
}
if (list1 == null) {
// What to return? Or throw exception.
}
if (list2 == null) {
// What to return? Or throw exception.
}
// do merging
while (!list1.isEmpty() && !list2.isEmpty()) {
// We know that both lists must have at least one element.
// 1. Figure out which list has the smaller element.
// 2. Remove that item from its list.
// 3. Add that item to the end of returnVal
}
// Now one or both lists are empty.
// Those items have to be added to "returnVal" as well.
return returnVal;
}
public static List<Integer> mergeRecursive(List<Integer> list1, List<Integer> list2) {
// do stuff
}
}Context
StackExchange Code Review Q#56479, answer score: 2
Revisions (0)
No revisions yet.