Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java Implementation of LFU #4

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/LFU/nbproject/private/
/LFU/build/
/LFU/dist/
40 changes: 40 additions & 0 deletions Java/FrequencyList.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package lfu;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lfu is a bad package name. But your choice though. Given your codebase is likely to be small you can go with it.

import java.util.LinkedList;
import java.util.ListIterator;

class FrequencyList<T> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

int frequency;
LinkedList<T> list;

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

public void add(T t){
list.add(t);
}

public void remove(T t){
list.remove(t);
}

public ListIterator<T> listIterator(int index){
return list.listIterator(index);
}

public int size(){
return list.size();
}

public T get(int index){
return list.get(index);
}

public int getFrequency(){
return frequency;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All code files should end with a new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be done!

144 changes: 144 additions & 0 deletions Java/LFU.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
package lfu;
import java.util.LinkedList;
import java.util.ListIterator;
import java.util.HashMap;

/**
* @author salman

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param <T>
*/

public class LFU<T> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sbshah97 sbshah97 May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private int MAX_SIZE = 2;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private final Integer.

try not to use primitives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do it. Any specific reason for avoiding primitives?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private LinkedList< FrequencyList<T> > lfuList;
private HashMap<T, Pair<Integer, ListIterator<T> > > nodeFrequencyMap;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I would probably make it Iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you would do that?

Copy link

@ashish1294 ashish1294 May 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private HashMap<Integer, ListIterator< FrequencyList<T> > > frequencyMap;
private int count = 0;

public void init() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do that now

lfuList = new LinkedList< FrequencyList<T> >();
nodeFrequencyMap = new HashMap<T, Pair<Integer, ListIterator<T> > >();

frequencyMap = new HashMap<Integer, ListIterator< FrequencyList<T> > >();
count = 0;
}

LFU(){
init();
}

LFU(int size){
MAX_SIZE = size;
init();
}

public void insert(T item){
if(count >= MAX_SIZE)
delete();

// If frequencyList is empty

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments in java should end with '.'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will be done!

if(!frequencyMap.containsKey(1)){
lfuList.listIterator(0).add(new FrequencyList(1));
frequencyMap.put(1, lfuList.listIterator(0));
}

ListIterator< FrequencyList<T> > fListIt = frequencyMap.get(1);
FrequencyList<T> frequencyList = fListIt.next();
fListIt.previous();
frequencyList.listIterator(frequencyList.size()).add(item);

// Update item frequency
nodeFrequencyMap.put(item, new Pair(1, frequencyList.listIterator(frequencyList.size() - 1)));

// Update Count
count++;
}

public boolean lookup(T item){

// If item does not exist
if(!nodeFrequencyMap.containsKey(item)){
insert(item);
return false;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there so much space here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random error using IDE. Will change that ASAP.

ListIterator<T> nodeIt = nodeFrequencyMap.get(item).second;
ListIterator< FrequencyList<T> > fListIt = frequencyMap.get(frequency);
FrequencyList<T> frequencyList = fListIt.next();

fListIt.previous();
System.out.println(item + " " + frequency + " " + frequencyList.getFrequency() + " " + fListIt);

// Remove Item from current frequencyList
nodeIt.next();
nodeIt.previous();
nodeIt.remove();

// Check if frequencyList is empty
if(frequencyList.size() == 0){
// Clean Up
frequencyMap.remove(frequencyList.getFrequency());
fListIt.remove();
}

// Add item to list frequency + 1
frequency++;

// If frequencyList is empty
if(!frequencyMap.containsKey(frequency)){
fListIt.add(new FrequencyList(frequency));
fListIt.previous();
frequencyMap.put(frequency, fListIt);
}

frequencyList = fListIt.next();
fListIt.previous();
frequencyList.listIterator(frequencyList.size()).add(item);

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

public T delete(){
ListIterator< FrequencyList<T> > fListIt = lfuList.listIterator(0);
FrequencyList<T> frequencyList = fListIt.next();

fListIt.previous();
T item = frequencyList.get(0);
ListIterator<T> nodeIt = nodeFrequencyMap.get(item).second;

nodeIt.next();
nodeIt.remove();
nodeFrequencyMap.remove(item);

if(frequencyList.size() == 0){
// Clean Up
frequencyMap.remove(frequencyList.getFrequency());
fListIt.remove();
}

// Update Count
count--;
System.out.println("Deleted "+item);
return item;
}

public static void main(String args[]) {
LFU<Integer> freqlfu = new LFU();

freqlfu.insert(10);

System.out.println(freqlfu.lookup(10));
System.out.println(freqlfu.lookup(10));
System.out.println(freqlfu.lookup(20));
System.out.flush();
System.out.println(freqlfu.lookup(10));
}
}
14 changes: 14 additions & 0 deletions Java/Pair.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package lfu;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class Pair<U, V> {
U first;
V second;

Pair(){}

Pair(U u, V v){
first = u;
second = v;
}

}