Refactoring lab (as577, srt15)

.classpath

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<classpath>
+	<classpathentry kind="src" path="src"/>
+	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/>
+	<classpathentry kind="output" path="bin"/>
+</classpath>

BinsCritique_as577.md

@@ -0,0 +1,13 @@
+Bins: Code Critique
+by Aditya Srinivasan (as577)
+
+The code Bins.java represents a Java solution to the problem described in the assignment. While the code is functional, there are ways in which the readability, testability, extensibility, and general design can be improved.
+
+READABILITY
+The comments detailing the purpose of each method are helpful in understanding where the helper methods are and where the main algorithm is. Further, smart naming of variables such as 'input', 'data', 'diskId', and classes such as 'Disk' and 'Bins', are very helpful in following the algorithm as opposed to arbitrary names such as 'foo' or 'i'. Conversely, the lack of any commenting in the body of the main algorithm obscures understanding. It would be helpful to describe the purpose of the for loop and if statements that are executed multiple times. Further, to someone unfamiliar with the concepts of worst-fit and worst-fit decreasing, a short explanatory overview of the program's function could be included as a comment at the header of the file. The code can be a lot more clear by establishing helpful variables at critical positions, and concise by removing useless variables and lines. For example, greater clarity can be achieved by introducing the idea of a 'diskId' before the priority queue is populated. This changes the seemingly arbitrary command "pq.add(new Disk(0))" to "pq.add(new Disk(diskId))". This convention is also later followed in the for-loop, making the connection between the two statements easier to connect and understand. An example of achieving more concise code is by eliminating the many "System.out.println()" commands and instead adding "\n" to the end of the preceding console-log statement. Further, with two blatant instances of duplicate code (the for-loops and console-log-sequences), additional helper methods can be introduced to shorten unnecessarily long code.
+
+TESTABILITY
+The code can be tested for bugs by using various kinds of input files wherein the format, data type, or length are altered. For example, a useful "test case" to determine whether the code is robust and versatile would be to change the format of the files from line-break separation to space-separation. In the current design of the program, this test case passes. If, however, the input data is not in the form of integers but instead doubles, the code fails. It is not simple to change the code to a Double paradigm from Integer since there are many appearances of 'int' and 'Integer' throughout Bins.java and Disk.java which would need to be changed. A simplification that can serve as a workaround is to implement a function that rounds all of the input data to the nearest integer for use throughout the algorithm. This would ensure that a test case with any decimal figures passes.
+
+EXTENSIBILITY
+There are plenty of examples of Smelly Code throughout the file. The first is the extremely long main method which contains repeated code and unexplained sections. This is a Bloater and can be resolved by Extracting Method. Another example, as mentioned before, is a Change Preventer in the Disk.java class. There is a strongly-rooted paradigm of integers in this class, making it difficult to work with doubles, floats, or longs in the event that the input is varied. There are also a few examples of Dispensables throughout the project. Firstly, there are two instances of duplicate code: the for-loops and the console-logs. Both of these can be extracted as methods to reduce the length and obscurity of the code. Secondly, there is dead code in the Disk.java file, wherein the constructor for an empty disk is never used - a disk ID is always specified. This section can be removed completely with no effect on the program (NOTE: in the refactored version of the program, a method was moved from Bins to Disk, and so the empty disk constructor was used to initialize an instance to refer to the method. However, in the original design, there is no need for the empty disk constructor). This can also be considered an example of Speculative Generality. As the code stands, there are Couplers in the form of Feature Envy in the main method, namely the for-loops which refer mostly to data from the Disk.java class. The for-loops can be extracted as a method and then moved to the Disk.java class so that the method is accessing data that belongs to the same class. As it stands, the code cannot be easily extended to compare a wider variety of fitting algorithms. This is because the algorithm being executed is in its entirety in the main method. By extracting the content of the main method into other methods, various fitting algorithms could be compared. There are dependencies in the code between methods and classes. One clear dependency is between Bins.java and Disk.java, since Bins.java uses Disk objects and other methods in the Disk class. Another dependency is between the main method and the readData method - if the latter were to fail then the former would also fail. There is also a dependency between the input file and the program. If the file were corrupted or incorrectly formatted, then the program would not execute properly. These dependencies are fairly simple to find since there is a clear reference to an external file, class, or method. It is difficult to completely remove the dependency without combining classes (which would result in unruly long code) but it is possible to reduce the amount of dependency. By moving methods with large dependencies into the appropriate class and reducing the number of parameters a method requires, the dependency becomes less.

src/Bins.java

@@ -1,92 +1,87 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.PriorityQueue;
 import java.util.Scanner;
+import java.util.stream.Collector;
 
 /**
  * Runs a number of algorithms that try to fit files onto disks.
  */
 public class Bins {
     public static final String DATA_FILE = "example.txt";
+    public static int diskId = 0;	// Initialize the global variables diskId and total, since they are used in the Disk class in the worstFit method.
+    public static int total = 0;
 
     /**
      * Reads list of integer data from the given input.
      *
      * @param input tied to an input source that contains space separated numbers
      * @return list of the numbers in the order they were read
      */
-    public List<Integer> readData (Scanner input) {
+    public List<Integer> readData(Scanner input) {
         List<Integer> results = new ArrayList<Integer>();
-        while (input.hasNext()) {
-            results.add(input.nextInt());
+        while(input.hasNext()) {
+        	if(!input.hasNextInt()) {	// This helps improve robustness of the program and prevents failure if there are non-integer entries.
+        		results.add((int) Math.round(input.nextDouble()));
+        	}
+        	else {
+        		results.add(input.nextInt());
+        	}
         }
         return results;
     }
-
+
+
+    /**
+     * Displays the contents of the priority queue and simultaneously empties it.
+     * 
+     * @param pq - PriorityQueue to be read
+     * @param method - Specification of what binning algorithm was used
+     */
+    public void displayResults(PriorityQueue<Disk> pq, String method) {
+    	System.out.println("\n" + method);	// The added "\n" reduces need for an extra println command.
+        System.out.println("number of pq used: " + pq.size());
+        while(!pq.isEmpty()) {
+            System.out.println(pq.poll());
+        }
+        // Removed a println command from here to shorten code and make more concise.
+    }
+
+    public void fitDisksAndPrint(List<Integer> list){
+    	list.stream().sorted((e1, e2) -> Integer.compare(e2, e1))
+        			 .forEach(e -> System.out.println(e));
+    }
+
     /**
      * The main program.
      */
     public static void main (String args[]) {
         Bins b = new Bins();
+        List<Integer> num = new ArrayList<Integer>();
+        num.addAll(Arrays.asList(1, 5, 4, 2, 7, 0, 3, 8));
+      //Collections.sort(data, Collections.reverseOrder());
+        b.fitDisksAndPrint(num);
+
+        Disk d = new Disk();	// Added an instance of the Disk class to access a newly added method (worstFit) from there. This cleans the code by eliminating Feature Envy (the Bins class using data mostly from the Disk class) and reducing dependency between the classes.
         Scanner input = new Scanner(Bins.class.getClassLoader().getResourceAsStream(DATA_FILE));
         List<Integer> data = b.readData(input);
-
         PriorityQueue<Disk> pq = new PriorityQueue<Disk>();
-        pq.add(new Disk(0));
-
-        int diskId = 1;
-        int total = 0;
-        for (Integer size : data) {
-            Disk d = pq.peek();
-            if (d.freeSpace() > size) {
-                pq.poll();
-                d.add(size);
-                pq.add(d);
-            } else {
-                Disk d2 = new Disk(diskId);
-                diskId++;
-                d2.add(size);
-                pq.add(d2);
-            }
-            total += size;
-        }
-
+
+        d.worstFit(data, pq);	// Calling the worst-fit algorithm on a given dataset of file sizes and PriorityQueue of disks. This cleans the code by reducing duplicate code (this method is called twice) and the position of the method in the Disk class reduces inter-class dependency.
+
         System.out.println("total size = " + total / 1000000.0 + "GB");
-        System.out.println();
-        System.out.println("worst-fit method");
-        System.out.println("number of pq used: " + pq.size());
-        while (!pq.isEmpty()) {
-            System.out.println(pq.poll());
-        }
-        System.out.println();
-
-        Collections.sort(data, Collections.reverseOrder());
-        pq.add(new Disk(0));
+
+        b.displayResults(pq, "worst-fit method");	// Calling a displaying method on a given PriorityQueue of disks with a specified binning algorithm name to be displayed. This reduces duplicate code (this method is called twice).
 
-        diskId = 1;
-        for (Integer size : data) {
-            Disk d = pq.peek();
-            if (d.freeSpace() >= size) {
-                pq.poll();
-                d.add(size);
-                pq.add(d);
-            } else {
-                Disk d2 = new Disk(diskId);
-                diskId++;
-                d2.add(size);
-                pq.add(d2);
-            }
-        }
-
-        System.out.println();
-        System.out.println("worst-fit decreasing method");
-        System.out.println("number of pq used: " + pq.size());
-        while (!pq.isEmpty()) {
-            System.out.println(pq.poll());
-        }
-        System.out.println();
+
+
+        d.worstFit(data, pq);	// Calling the worst-fit algorithm on a given dataset and PriorityQueue for the second time.
+
+        b.displayResults(pq, "worst-fit decreasing method");	// Calling the displaying method on a given PriorityQueue for the second time.
     }
 }

src/Disk.java

@@ -1,5 +1,6 @@
 import java.util.ArrayList;
 import java.util.List;
+import java.util.PriorityQueue;
 
 /**
  * Represents a collection of files; how many it can hold is limited by its capacity.
@@ -9,25 +10,53 @@ public class Disk implements Comparable<Disk> {
     private int mySize;
     private int myCapacity;
     private List<Integer> myFiles;
+    private static final int CAPACITY = 1000000;	// Allows user to change parameter once instead of twice as in the original design (Change Preventer).
 
     /**
-     * Create an empty Disk.
+     * Create an empty Disk.	// Removing this method in the original design would have no impact as it was never used in practice and was considered dead code. HOWEVER, in the refactored design, a method was moved and so the empty constructor is used to initialize an instance of the Disk class.
      */
-    public Disk () {
+    public Disk() {
         mySize = 0;
-        myCapacity = 1000000;
+        myCapacity = CAPACITY;
         myFiles = new ArrayList<Integer>();
     }
+
 
     /**
      * Create an empty Disk with the given ID.
      */
     public Disk (int id) {
         myId = id;
         mySize = 0;
-        myCapacity = 1000000;
+        myCapacity = CAPACITY;
         myFiles = new ArrayList<Integer>();
     }
+
+    /**
+     * Executes the worst-fit bin algorithm on a specified PriorityQueue of disks and input data
+     * 
+     * @param data - the input file sizes as a List
+     * @param pq - the PriorityQueue of disks
+     */
+    public void worstFit(List<Integer> data, PriorityQueue<Disk> pq) {
+    	Bins.diskId = 0;
+    	pq.add(new Disk(Bins.diskId));	// Using diskId instead of '0' for better understanding of the parameter.
+    	for(Integer size : data) {
+            Disk d = pq.peek();
+            if (d.freeSpace() >= size) {
+                pq.poll();
+                d.add(size);
+                pq.add(d);
+            } else {
+            	Bins.diskId++;
+                Disk d2 = new Disk(Bins.diskId);
+                d2.add(size);
+                pq.add(d2);
+            }
+            Bins.total += size;
+        }
+    }
+
 
     /**
      * @return amount of free space available on this disk