Java Implementation of LFU #4

sbshah97 · 2017-04-29T16:09:29Z

Issue Resolved: #4

Not all test cases covered! Basic test cases are all covered.
Kindly do let us know for suggestions for the same!
Code has been made as a NetBeans IDE Project! Code compiles and runs in the same! Though src directory contains all the Java Source files!

@adeepkit01 @mohitreddy1996 @chinmaydd Kindly review!

P.S. You can squash the commits if you want. 😄

Documentation changes

chinmaydd · 2017-04-30T06:49:32Z

LFU/src/lfu/FrequencyList.java

+	FrequencyList(){
+            list = new LinkedList<T>();
+        }
+	FrequencyList(int f){


Use similar spacing between functions

Agreed! 😄

chinmaydd · 2017-04-30T06:50:52Z

LFU/src/lfu/LFU.java

+		// Update item frequency
+		nodeIt = frequencyList.listIterator(frequencyList.size() - 1);
+		nodeFrequencyMap.put(item, new Pair(frequency, nodeIt));
+                return true;


Lot of indentation issues.

Sorry will fix that ASAP!

chinmaydd · 2017-04-30T06:55:51Z

LFU/src/lfu/FrequencyList.java

+		return list.listIterator(index);
+	}
+
+	public int getFrequency(){


Maintain an ordering for getter and setter functions. Looks better ;)

OKay that somehow makes sense as well!

chinmaydd · 2017-04-30T06:56:00Z

Write some test cases for your work. It is tough to evaluate the PR only based on source of the logic.
Try not including "merge" commits into your PR. The maintainers will have additional work in cherry picking the useful ones ;)
Do we really need project files? I guess only the Java ones should work.

sbshah97 · 2017-04-30T11:39:00Z

@chinmaydd that's why I suggested to squash the commits! 😄

sbshah97 · 2017-05-01T12:25:22Z

@chinmaydd will do the testcases part by tomorrow. Kindly take a look at the existing code for now!
@mohitreddy1996 @adeepkit01 If possible please do review this as well!

mohitreddy1996 · 2017-05-02T19:43:16Z

@salman-bhai Please use JUnit or some other testing tool to test your code with given test cases. Please do add the JUnit files.

sbshah97 · 2017-05-05T12:22:12Z

Alright @mohitreddy1996 will do that tonight!

ashish1294

Plenty to address for now.

ashish1294 · 2017-05-06T09:15:06Z

Java/Pair.java

@@ -0,0 +1,14 @@
+package lfu;


Try not to define common types like this on your own (as they will be prone to errors and re-work). There is a well known principle in programming called the DRY (DO NOT REPEAT YOURSELF).

Pairs are usually discouraged. Why not use an Auto_Value class here ?
This will also be a good learning for you.

ashish1294 · 2017-05-06T09:15:53Z

Java/LFU.java

+
+public class LFU<T> {
+
+	private int MAX_SIZE = 2;


This should be private final Integer.

try not to use primitives.

Will do it. Any specific reason for avoiding primitives?

Unless you are writing super-efficient low level programs, primitives don't have any real benefit. Primitives don't fit well in larger scheme of things. You will realize this when you work on Java enough.

For future references, there is nothing called "will do". You should consider making the change and only then reverting back. This ensures that code quality can be improved iteratively.

ashish1294 · 2017-05-06T09:16:43Z

Java/LFU.java

+	private HashMap<Integer, ListIterator< FrequencyList<T> > > frequencyMap;
+	private int count = 0;
+
+	public void init() {		


If the sole function of this init() is to be called from the constructor, then place this code in constructor itself.

Will do that now

ashish1294 · 2017-05-06T09:17:25Z

Java/LFU.java

+ * @param <T>
+ */
+
+public class LFU<T> {


LFU is a bad class name. LeastFrequetlyUsedPage is a better name.

Umm aren't long names not advised as the name of a class? LFU is a bad name though. Coulldn't think of an alternative though!

Where did you read that Long names are not advisable ? Until 6 Words, you need not worry about long names. In my opinion, Java class name should be as descriptive as possible. If it's descriptive enough, it won't require any documentation.

In practice, I have personally written classes with names as long as 10 words. So it's fine.

ashish1294 · 2017-05-06T09:18:35Z

Java/LFU.java

+		if(count >= MAX_SIZE)
+			delete();
+
+		// If frequencyList is empty


Comments in java should end with '.'

OK, will be done!

ashish1294 · 2017-05-06T09:19:51Z

Java/FrequencyList.java

+import java.util.LinkedList;
+import java.util.ListIterator;
+
+class FrequencyList<T> {


Most of this java code has too many lint / formatting errors. Did you try running a java linter on it ?

Umm no. Didn't know what a Java linter was till now. Will do it now!

ashish1294 · 2017-05-06T09:20:08Z

Java/FrequencyList.java

+	public int getFrequency(){
+		return frequency;
+	}
+}


All code files should end with a new line

Will be done!

ashish1294 · 2017-05-06T09:20:44Z

Java/LFU.java

+import java.util.HashMap;
+
+/**
+ * @author salman


If you do plan to add a javadoc, then add something useful here. And not just an @author

Could you suggest something in this reference? Something which can be suited just for this.

Since this is an instanciable class, you should use a singular noun describing what one instance of this class represents. For example

An element in cache that is least frequently used.

Honestly, I am skeptical about the name and responsibilities of this class.

ashish1294 · 2017-05-06T09:21:49Z

Java/LFU.java

+
+		// If item exists
+		// Get FrequencyList of item
+		int frequency          = nodeFrequencyMap.get(item).first;


Why is there so much space here ?

Random error using IDE. Will change that ASAP.

ashish1294 · 2017-05-06T09:22:38Z

Java/LFU.java

+	private int MAX_SIZE = 2;
+
+	private LinkedList< FrequencyList<T> > lfuList;
+	private HashMap<T, Pair<Integer, ListIterator<T> > > nodeFrequencyMap;


If I were you, I would never use ListIterator directory.

I would probably make it Iterable

Any reason why you would do that?

You can only iterate through an interator once. But if you take an iterable, you can easily call .iterator() on it, to get it's corresponding iterator.

Plus, ListIterator is too restrictive. I don't think you need any aspect of ListIterator that is not available in Iterator itself.

If you want gurantee over order of iteration, you should use Guava library's ImmutableCollection.

Insert, Locate, Delete Methods Added Changes made Documentation update (WebClub-NITK#3) * Documentation update * Changes made as suggested! New exception yet to be handled Code reorganised as per review Initial Java code for testcases added

Please Change if anything requires to be changed. Couldn't find proper convention for writing test cases.

sbshah97 and others added 5 commits April 22, 2017 15:17

Initial commit

f276254

Insert, Locate, Delete Methods Added

faf17f4

Changes made

080073c

Merge branch 'master' of https://github.com/WebClub-NITK/LFU into java

1ef2721

Documentation changes

New exception yet to be handled

87de674

chinmaydd reviewed Apr 30, 2017

View reviewed changes

Code reorganised as per review

d76210e

mohitreddy1996 requested review from adeepkit01 and mohitreddy1996 May 3, 2017 04:38

ashish1294 reviewed May 6, 2017

View reviewed changes

sbshah97 and others added 4 commits July 17, 2017 01:23

New Testcases added

bff6905

Initial commit

a2f5ac7

Insert, Locate, Delete Methods Added Changes made Documentation update (WebClub-NITK#3) * Documentation update * Changes made as suggested! New exception yet to be handled Code reorganised as per review Initial Java code for testcases added

Testcases Added.

87d76c1

Please Change if anything requires to be changed. Couldn't find proper convention for writing test cases.

New Java documentation added

1674686

Java Implementation of LFU #4

Are you sure you want to change the base?

Java Implementation of LFU #4

Conversation

sbshah97 commented Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaydd commented Apr 30, 2017 • edited Loading

sbshah97 commented Apr 30, 2017

sbshah97 commented May 1, 2017

mohitreddy1996 commented May 2, 2017

sbshah97 commented May 5, 2017

ashish1294 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbshah97 May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish1294 May 7, 2017 • edited Loading

Choose a reason for hiding this comment

sbshah97 commented Apr 29, 2017 •

edited

Loading

chinmaydd commented Apr 30, 2017 •

edited

Loading

sbshah97 May 6, 2017 •

edited

Loading

ashish1294 May 7, 2017 •

edited

Loading