LRU implementation in C++ using a LinkedList
$begingroup$
Explanation : I am adding new elements in the list until the window runs out. Head will have the earliest element and tail will have the most recent element. Once the window is over, every addition requires deletion of the very first element or least used in the list which was temporally inserted a lot earlier than other elements. An access to an existing page in the list brings it to the end of the list signifying most recent use.
List end represents most recent element in the cache.
List head represents earliest of all the elements in the cache.
Link.h
#ifndef LINKLIST_LINK_H
#define LINKLIST_LINK_H
#include <iosfwd>
class Link{
private:
int value;
Link* next = nullptr;
public:
Link();
explicit Link(int key);
int getValue() const;
void setValue(int value);
Link *getNext() const;
void setNext(Link *next);
friend void operator<<(std::ostream& os, const Link &link);
};
#endif //LINKLIST_LINK_H
Link.cpp
#include "Link.h"
#include <iostream>
Link::Link() {
}
Link::Link(int key) {
this->setValue(key);
}
int Link::getValue() const {
return value;
}
void Link::setValue(int value) {
Link::value = value;
}
Link *Link::getNext() const {
return next;
}
void Link::setNext(Link *next) {
Link::next = next;
}
void operator<<(std::ostream& os, const Link &link){
os<<link.getValue()<<" ";
}
LinkList.h
#ifndef LINKLIST_LINKLIST_H
#define LINKLIST_LINKLIST_H
#include <vector>
#include "Link.h"
class LinkList{
protected:
Link* head = nullptr;
Link* tail = nullptr;
std::vector<void* >linksAddresses;
public:
virtual ~LinkList();
void insert(int key);
void push_back(int key);
void push_front(int key);
bool deleteKey(int key);
bool findKey(int key);
void insert_sorted(int key);
friend void operator<<(std::ostream& os, const LinkList &linkList);
void getLinksAddresses();
void printReverse() const;
void do_reverse();
Link* delete_at_pos(int n);
};
#endif //LINKLIST_LINKLIST_H
LinkList.cpp
#include <iostream>
#include "LinkList.h"
void LinkList::insert(int key) {
push_back(key);
}
void LinkList::push_front(int key) {
Link *newNode = new Link(key);
if (head == nullptr) {
head = newNode;
tail = newNode;
} else {
newNode->setNext(head);
head = newNode;
}
//std::cout << "Inserted " << key <<" address : "<<&newNode << " at frontn";
}
void LinkList::push_back(int key) {
Link *newNode = new Link(key);
if (tail == nullptr) {
head = newNode;
tail = newNode;
} else {
tail->setNext(newNode);
tail = newNode;
}
//std::cout << "Inserted " << key << " at backn";
}
bool LinkList::deleteKey(int key) {
Link *curr = head;
Link *prev = head;
if (head != nullptr && head->getValue() == key) {
Link *temp = head;
head = head->getNext();
if(head == nullptr)
tail = nullptr;
delete temp;
std::cout << "Deleted " << key << "n";
return true;
}
while (curr != nullptr && curr->getValue() != key) {
prev = curr;
curr = curr->getNext();
}
if (curr == nullptr) {
std::cout << "To delete Key " << key << " doesn't existn";
return false;
} else {
prev->setNext(curr->getNext());
delete curr;
std::cout << "Deleted " << key << "n";
return true;
}
}
bool LinkList::findKey(int key) {
Link *current = head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current != nullptr;
}
LinkList::~LinkList() {
Link *next;
Link *curr = head;
while (curr != nullptr) {
next = curr->getNext();
delete curr;
curr = next;
}
//std::cout<<"Memory freedn";
}
void operator<<(std::ostream &os, const LinkList &linkList) {
Link *curr = linkList.head;
os << "List is : ";
while (curr != nullptr) {
os << *curr;
curr = curr->getNext();
}
os << 'n';
}
void LinkList::insert_sorted(int key) {
//TODO
}
void LinkList::getLinksAddresses() {
while (head != nullptr){
linksAddresses.push_back(&head);
std::cout<<&head<<" "<<head->getValue()<<"n";
head = head->getNext();
}
}
void reverse(Link* head){
if(head!= nullptr){
reverse(head->getNext());
std::cout<<head->getValue()<<" ";
}
}
void LinkList::printReverse() const {
reverse(head);
std::cout<<"n";
}
void LinkList::do_reverse() {
Link* prev = nullptr;
Link* curr = head;
Link* next = curr->getNext();
while (curr){
}
}
Link* LinkList::delete_at_pos(int n)
{
if(n <=0)
return head;
int count = 1;
Link* curr = head, *prev = nullptr;;
while(curr!=nullptr){
if(count == n)
break;
prev = curr;
curr = curr->getNext();
count++;
}
if(curr!=nullptr){
Link* temp = curr;
if(curr == head){
head = head->getNext();
}else{
prev->setNext(curr->getNext());
}
delete temp;
}
return head;
}
LRU.hpp
#ifndef LINKLIST_LRU_HPP
#define LINKLIST_LRU_HPP
#include <iostream>
#include "LinkList.h"
class LRU : public LinkList{
private:
const int MAX = 4;
int max_len = 0;
Link* findPage(int key){
Link *current = LinkList::head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current;
}
public:
void insert(int key) override{
if(MAX > 0) {
Link *page = findPage(key);
if (page != nullptr) {
access(page->getValue());
return;
}
if (max_len >= MAX) {
deleteKey(LinkList::head->getValue());
max_len--;
}
push_back(key);
max_len++;
}
}
bool access(int key){
Link *current = findPage(key);
if(current == nullptr)
return false;
else{
int val = current->getValue();
deleteKey(val);
max_len--;
insert(val);
return true;
}
}
};
#endif //LINKLIST_LRU_HPP
main.cpp
#include "LinkList.h"
#include "LRU.hpp"
#include <iostream>
void testingLinkedLRU();
int main() {
testingLinkedLRU();
return 0;
}
void testingLinkedLRU(){
LRU lru;
std::cout<<lru;
lru.insert(1);
std::cout<<lru;
lru.insert(2);
std::cout<<lru;
lru.insert(3);
std::cout<<lru;
lru.insert(4);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(6);
std::cout<<lru;
lru.access(3);
std::cout<<lru;
lru.insert(-5);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(50);
std::cout<<lru;
lru.access(5);
std::cout<<lru;
lru.access(1);
std::cout<<lru;
lru.access(-5);
std::cout<<lru;
}
c++ linked-list reinventing-the-wheel memory-management
$endgroup$
add a comment |
$begingroup$
Explanation : I am adding new elements in the list until the window runs out. Head will have the earliest element and tail will have the most recent element. Once the window is over, every addition requires deletion of the very first element or least used in the list which was temporally inserted a lot earlier than other elements. An access to an existing page in the list brings it to the end of the list signifying most recent use.
List end represents most recent element in the cache.
List head represents earliest of all the elements in the cache.
Link.h
#ifndef LINKLIST_LINK_H
#define LINKLIST_LINK_H
#include <iosfwd>
class Link{
private:
int value;
Link* next = nullptr;
public:
Link();
explicit Link(int key);
int getValue() const;
void setValue(int value);
Link *getNext() const;
void setNext(Link *next);
friend void operator<<(std::ostream& os, const Link &link);
};
#endif //LINKLIST_LINK_H
Link.cpp
#include "Link.h"
#include <iostream>
Link::Link() {
}
Link::Link(int key) {
this->setValue(key);
}
int Link::getValue() const {
return value;
}
void Link::setValue(int value) {
Link::value = value;
}
Link *Link::getNext() const {
return next;
}
void Link::setNext(Link *next) {
Link::next = next;
}
void operator<<(std::ostream& os, const Link &link){
os<<link.getValue()<<" ";
}
LinkList.h
#ifndef LINKLIST_LINKLIST_H
#define LINKLIST_LINKLIST_H
#include <vector>
#include "Link.h"
class LinkList{
protected:
Link* head = nullptr;
Link* tail = nullptr;
std::vector<void* >linksAddresses;
public:
virtual ~LinkList();
void insert(int key);
void push_back(int key);
void push_front(int key);
bool deleteKey(int key);
bool findKey(int key);
void insert_sorted(int key);
friend void operator<<(std::ostream& os, const LinkList &linkList);
void getLinksAddresses();
void printReverse() const;
void do_reverse();
Link* delete_at_pos(int n);
};
#endif //LINKLIST_LINKLIST_H
LinkList.cpp
#include <iostream>
#include "LinkList.h"
void LinkList::insert(int key) {
push_back(key);
}
void LinkList::push_front(int key) {
Link *newNode = new Link(key);
if (head == nullptr) {
head = newNode;
tail = newNode;
} else {
newNode->setNext(head);
head = newNode;
}
//std::cout << "Inserted " << key <<" address : "<<&newNode << " at frontn";
}
void LinkList::push_back(int key) {
Link *newNode = new Link(key);
if (tail == nullptr) {
head = newNode;
tail = newNode;
} else {
tail->setNext(newNode);
tail = newNode;
}
//std::cout << "Inserted " << key << " at backn";
}
bool LinkList::deleteKey(int key) {
Link *curr = head;
Link *prev = head;
if (head != nullptr && head->getValue() == key) {
Link *temp = head;
head = head->getNext();
if(head == nullptr)
tail = nullptr;
delete temp;
std::cout << "Deleted " << key << "n";
return true;
}
while (curr != nullptr && curr->getValue() != key) {
prev = curr;
curr = curr->getNext();
}
if (curr == nullptr) {
std::cout << "To delete Key " << key << " doesn't existn";
return false;
} else {
prev->setNext(curr->getNext());
delete curr;
std::cout << "Deleted " << key << "n";
return true;
}
}
bool LinkList::findKey(int key) {
Link *current = head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current != nullptr;
}
LinkList::~LinkList() {
Link *next;
Link *curr = head;
while (curr != nullptr) {
next = curr->getNext();
delete curr;
curr = next;
}
//std::cout<<"Memory freedn";
}
void operator<<(std::ostream &os, const LinkList &linkList) {
Link *curr = linkList.head;
os << "List is : ";
while (curr != nullptr) {
os << *curr;
curr = curr->getNext();
}
os << 'n';
}
void LinkList::insert_sorted(int key) {
//TODO
}
void LinkList::getLinksAddresses() {
while (head != nullptr){
linksAddresses.push_back(&head);
std::cout<<&head<<" "<<head->getValue()<<"n";
head = head->getNext();
}
}
void reverse(Link* head){
if(head!= nullptr){
reverse(head->getNext());
std::cout<<head->getValue()<<" ";
}
}
void LinkList::printReverse() const {
reverse(head);
std::cout<<"n";
}
void LinkList::do_reverse() {
Link* prev = nullptr;
Link* curr = head;
Link* next = curr->getNext();
while (curr){
}
}
Link* LinkList::delete_at_pos(int n)
{
if(n <=0)
return head;
int count = 1;
Link* curr = head, *prev = nullptr;;
while(curr!=nullptr){
if(count == n)
break;
prev = curr;
curr = curr->getNext();
count++;
}
if(curr!=nullptr){
Link* temp = curr;
if(curr == head){
head = head->getNext();
}else{
prev->setNext(curr->getNext());
}
delete temp;
}
return head;
}
LRU.hpp
#ifndef LINKLIST_LRU_HPP
#define LINKLIST_LRU_HPP
#include <iostream>
#include "LinkList.h"
class LRU : public LinkList{
private:
const int MAX = 4;
int max_len = 0;
Link* findPage(int key){
Link *current = LinkList::head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current;
}
public:
void insert(int key) override{
if(MAX > 0) {
Link *page = findPage(key);
if (page != nullptr) {
access(page->getValue());
return;
}
if (max_len >= MAX) {
deleteKey(LinkList::head->getValue());
max_len--;
}
push_back(key);
max_len++;
}
}
bool access(int key){
Link *current = findPage(key);
if(current == nullptr)
return false;
else{
int val = current->getValue();
deleteKey(val);
max_len--;
insert(val);
return true;
}
}
};
#endif //LINKLIST_LRU_HPP
main.cpp
#include "LinkList.h"
#include "LRU.hpp"
#include <iostream>
void testingLinkedLRU();
int main() {
testingLinkedLRU();
return 0;
}
void testingLinkedLRU(){
LRU lru;
std::cout<<lru;
lru.insert(1);
std::cout<<lru;
lru.insert(2);
std::cout<<lru;
lru.insert(3);
std::cout<<lru;
lru.insert(4);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(6);
std::cout<<lru;
lru.access(3);
std::cout<<lru;
lru.insert(-5);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(50);
std::cout<<lru;
lru.access(5);
std::cout<<lru;
lru.access(1);
std::cout<<lru;
lru.access(-5);
std::cout<<lru;
}
c++ linked-list reinventing-the-wheel memory-management
$endgroup$
add a comment |
$begingroup$
Explanation : I am adding new elements in the list until the window runs out. Head will have the earliest element and tail will have the most recent element. Once the window is over, every addition requires deletion of the very first element or least used in the list which was temporally inserted a lot earlier than other elements. An access to an existing page in the list brings it to the end of the list signifying most recent use.
List end represents most recent element in the cache.
List head represents earliest of all the elements in the cache.
Link.h
#ifndef LINKLIST_LINK_H
#define LINKLIST_LINK_H
#include <iosfwd>
class Link{
private:
int value;
Link* next = nullptr;
public:
Link();
explicit Link(int key);
int getValue() const;
void setValue(int value);
Link *getNext() const;
void setNext(Link *next);
friend void operator<<(std::ostream& os, const Link &link);
};
#endif //LINKLIST_LINK_H
Link.cpp
#include "Link.h"
#include <iostream>
Link::Link() {
}
Link::Link(int key) {
this->setValue(key);
}
int Link::getValue() const {
return value;
}
void Link::setValue(int value) {
Link::value = value;
}
Link *Link::getNext() const {
return next;
}
void Link::setNext(Link *next) {
Link::next = next;
}
void operator<<(std::ostream& os, const Link &link){
os<<link.getValue()<<" ";
}
LinkList.h
#ifndef LINKLIST_LINKLIST_H
#define LINKLIST_LINKLIST_H
#include <vector>
#include "Link.h"
class LinkList{
protected:
Link* head = nullptr;
Link* tail = nullptr;
std::vector<void* >linksAddresses;
public:
virtual ~LinkList();
void insert(int key);
void push_back(int key);
void push_front(int key);
bool deleteKey(int key);
bool findKey(int key);
void insert_sorted(int key);
friend void operator<<(std::ostream& os, const LinkList &linkList);
void getLinksAddresses();
void printReverse() const;
void do_reverse();
Link* delete_at_pos(int n);
};
#endif //LINKLIST_LINKLIST_H
LinkList.cpp
#include <iostream>
#include "LinkList.h"
void LinkList::insert(int key) {
push_back(key);
}
void LinkList::push_front(int key) {
Link *newNode = new Link(key);
if (head == nullptr) {
head = newNode;
tail = newNode;
} else {
newNode->setNext(head);
head = newNode;
}
//std::cout << "Inserted " << key <<" address : "<<&newNode << " at frontn";
}
void LinkList::push_back(int key) {
Link *newNode = new Link(key);
if (tail == nullptr) {
head = newNode;
tail = newNode;
} else {
tail->setNext(newNode);
tail = newNode;
}
//std::cout << "Inserted " << key << " at backn";
}
bool LinkList::deleteKey(int key) {
Link *curr = head;
Link *prev = head;
if (head != nullptr && head->getValue() == key) {
Link *temp = head;
head = head->getNext();
if(head == nullptr)
tail = nullptr;
delete temp;
std::cout << "Deleted " << key << "n";
return true;
}
while (curr != nullptr && curr->getValue() != key) {
prev = curr;
curr = curr->getNext();
}
if (curr == nullptr) {
std::cout << "To delete Key " << key << " doesn't existn";
return false;
} else {
prev->setNext(curr->getNext());
delete curr;
std::cout << "Deleted " << key << "n";
return true;
}
}
bool LinkList::findKey(int key) {
Link *current = head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current != nullptr;
}
LinkList::~LinkList() {
Link *next;
Link *curr = head;
while (curr != nullptr) {
next = curr->getNext();
delete curr;
curr = next;
}
//std::cout<<"Memory freedn";
}
void operator<<(std::ostream &os, const LinkList &linkList) {
Link *curr = linkList.head;
os << "List is : ";
while (curr != nullptr) {
os << *curr;
curr = curr->getNext();
}
os << 'n';
}
void LinkList::insert_sorted(int key) {
//TODO
}
void LinkList::getLinksAddresses() {
while (head != nullptr){
linksAddresses.push_back(&head);
std::cout<<&head<<" "<<head->getValue()<<"n";
head = head->getNext();
}
}
void reverse(Link* head){
if(head!= nullptr){
reverse(head->getNext());
std::cout<<head->getValue()<<" ";
}
}
void LinkList::printReverse() const {
reverse(head);
std::cout<<"n";
}
void LinkList::do_reverse() {
Link* prev = nullptr;
Link* curr = head;
Link* next = curr->getNext();
while (curr){
}
}
Link* LinkList::delete_at_pos(int n)
{
if(n <=0)
return head;
int count = 1;
Link* curr = head, *prev = nullptr;;
while(curr!=nullptr){
if(count == n)
break;
prev = curr;
curr = curr->getNext();
count++;
}
if(curr!=nullptr){
Link* temp = curr;
if(curr == head){
head = head->getNext();
}else{
prev->setNext(curr->getNext());
}
delete temp;
}
return head;
}
LRU.hpp
#ifndef LINKLIST_LRU_HPP
#define LINKLIST_LRU_HPP
#include <iostream>
#include "LinkList.h"
class LRU : public LinkList{
private:
const int MAX = 4;
int max_len = 0;
Link* findPage(int key){
Link *current = LinkList::head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current;
}
public:
void insert(int key) override{
if(MAX > 0) {
Link *page = findPage(key);
if (page != nullptr) {
access(page->getValue());
return;
}
if (max_len >= MAX) {
deleteKey(LinkList::head->getValue());
max_len--;
}
push_back(key);
max_len++;
}
}
bool access(int key){
Link *current = findPage(key);
if(current == nullptr)
return false;
else{
int val = current->getValue();
deleteKey(val);
max_len--;
insert(val);
return true;
}
}
};
#endif //LINKLIST_LRU_HPP
main.cpp
#include "LinkList.h"
#include "LRU.hpp"
#include <iostream>
void testingLinkedLRU();
int main() {
testingLinkedLRU();
return 0;
}
void testingLinkedLRU(){
LRU lru;
std::cout<<lru;
lru.insert(1);
std::cout<<lru;
lru.insert(2);
std::cout<<lru;
lru.insert(3);
std::cout<<lru;
lru.insert(4);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(6);
std::cout<<lru;
lru.access(3);
std::cout<<lru;
lru.insert(-5);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(50);
std::cout<<lru;
lru.access(5);
std::cout<<lru;
lru.access(1);
std::cout<<lru;
lru.access(-5);
std::cout<<lru;
}
c++ linked-list reinventing-the-wheel memory-management
$endgroup$
Explanation : I am adding new elements in the list until the window runs out. Head will have the earliest element and tail will have the most recent element. Once the window is over, every addition requires deletion of the very first element or least used in the list which was temporally inserted a lot earlier than other elements. An access to an existing page in the list brings it to the end of the list signifying most recent use.
List end represents most recent element in the cache.
List head represents earliest of all the elements in the cache.
Link.h
#ifndef LINKLIST_LINK_H
#define LINKLIST_LINK_H
#include <iosfwd>
class Link{
private:
int value;
Link* next = nullptr;
public:
Link();
explicit Link(int key);
int getValue() const;
void setValue(int value);
Link *getNext() const;
void setNext(Link *next);
friend void operator<<(std::ostream& os, const Link &link);
};
#endif //LINKLIST_LINK_H
Link.cpp
#include "Link.h"
#include <iostream>
Link::Link() {
}
Link::Link(int key) {
this->setValue(key);
}
int Link::getValue() const {
return value;
}
void Link::setValue(int value) {
Link::value = value;
}
Link *Link::getNext() const {
return next;
}
void Link::setNext(Link *next) {
Link::next = next;
}
void operator<<(std::ostream& os, const Link &link){
os<<link.getValue()<<" ";
}
LinkList.h
#ifndef LINKLIST_LINKLIST_H
#define LINKLIST_LINKLIST_H
#include <vector>
#include "Link.h"
class LinkList{
protected:
Link* head = nullptr;
Link* tail = nullptr;
std::vector<void* >linksAddresses;
public:
virtual ~LinkList();
void insert(int key);
void push_back(int key);
void push_front(int key);
bool deleteKey(int key);
bool findKey(int key);
void insert_sorted(int key);
friend void operator<<(std::ostream& os, const LinkList &linkList);
void getLinksAddresses();
void printReverse() const;
void do_reverse();
Link* delete_at_pos(int n);
};
#endif //LINKLIST_LINKLIST_H
LinkList.cpp
#include <iostream>
#include "LinkList.h"
void LinkList::insert(int key) {
push_back(key);
}
void LinkList::push_front(int key) {
Link *newNode = new Link(key);
if (head == nullptr) {
head = newNode;
tail = newNode;
} else {
newNode->setNext(head);
head = newNode;
}
//std::cout << "Inserted " << key <<" address : "<<&newNode << " at frontn";
}
void LinkList::push_back(int key) {
Link *newNode = new Link(key);
if (tail == nullptr) {
head = newNode;
tail = newNode;
} else {
tail->setNext(newNode);
tail = newNode;
}
//std::cout << "Inserted " << key << " at backn";
}
bool LinkList::deleteKey(int key) {
Link *curr = head;
Link *prev = head;
if (head != nullptr && head->getValue() == key) {
Link *temp = head;
head = head->getNext();
if(head == nullptr)
tail = nullptr;
delete temp;
std::cout << "Deleted " << key << "n";
return true;
}
while (curr != nullptr && curr->getValue() != key) {
prev = curr;
curr = curr->getNext();
}
if (curr == nullptr) {
std::cout << "To delete Key " << key << " doesn't existn";
return false;
} else {
prev->setNext(curr->getNext());
delete curr;
std::cout << "Deleted " << key << "n";
return true;
}
}
bool LinkList::findKey(int key) {
Link *current = head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current != nullptr;
}
LinkList::~LinkList() {
Link *next;
Link *curr = head;
while (curr != nullptr) {
next = curr->getNext();
delete curr;
curr = next;
}
//std::cout<<"Memory freedn";
}
void operator<<(std::ostream &os, const LinkList &linkList) {
Link *curr = linkList.head;
os << "List is : ";
while (curr != nullptr) {
os << *curr;
curr = curr->getNext();
}
os << 'n';
}
void LinkList::insert_sorted(int key) {
//TODO
}
void LinkList::getLinksAddresses() {
while (head != nullptr){
linksAddresses.push_back(&head);
std::cout<<&head<<" "<<head->getValue()<<"n";
head = head->getNext();
}
}
void reverse(Link* head){
if(head!= nullptr){
reverse(head->getNext());
std::cout<<head->getValue()<<" ";
}
}
void LinkList::printReverse() const {
reverse(head);
std::cout<<"n";
}
void LinkList::do_reverse() {
Link* prev = nullptr;
Link* curr = head;
Link* next = curr->getNext();
while (curr){
}
}
Link* LinkList::delete_at_pos(int n)
{
if(n <=0)
return head;
int count = 1;
Link* curr = head, *prev = nullptr;;
while(curr!=nullptr){
if(count == n)
break;
prev = curr;
curr = curr->getNext();
count++;
}
if(curr!=nullptr){
Link* temp = curr;
if(curr == head){
head = head->getNext();
}else{
prev->setNext(curr->getNext());
}
delete temp;
}
return head;
}
LRU.hpp
#ifndef LINKLIST_LRU_HPP
#define LINKLIST_LRU_HPP
#include <iostream>
#include "LinkList.h"
class LRU : public LinkList{
private:
const int MAX = 4;
int max_len = 0;
Link* findPage(int key){
Link *current = LinkList::head;
while (current != nullptr && current->getValue() != key)
current = current->getNext();
return current;
}
public:
void insert(int key) override{
if(MAX > 0) {
Link *page = findPage(key);
if (page != nullptr) {
access(page->getValue());
return;
}
if (max_len >= MAX) {
deleteKey(LinkList::head->getValue());
max_len--;
}
push_back(key);
max_len++;
}
}
bool access(int key){
Link *current = findPage(key);
if(current == nullptr)
return false;
else{
int val = current->getValue();
deleteKey(val);
max_len--;
insert(val);
return true;
}
}
};
#endif //LINKLIST_LRU_HPP
main.cpp
#include "LinkList.h"
#include "LRU.hpp"
#include <iostream>
void testingLinkedLRU();
int main() {
testingLinkedLRU();
return 0;
}
void testingLinkedLRU(){
LRU lru;
std::cout<<lru;
lru.insert(1);
std::cout<<lru;
lru.insert(2);
std::cout<<lru;
lru.insert(3);
std::cout<<lru;
lru.insert(4);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(6);
std::cout<<lru;
lru.access(3);
std::cout<<lru;
lru.insert(-5);
std::cout<<lru;
lru.insert(5);
std::cout<<lru;
lru.insert(50);
std::cout<<lru;
lru.access(5);
std::cout<<lru;
lru.access(1);
std::cout<<lru;
lru.access(-5);
std::cout<<lru;
}
c++ linked-list reinventing-the-wheel memory-management
c++ linked-list reinventing-the-wheel memory-management
edited Feb 16 at 20:39
piepi
asked Feb 16 at 20:23
piepipiepi
441316
441316
add a comment |
add a comment |
3 Answers
3
active
oldest
votes
$begingroup$
It is misleading that the same field in
Link
is sometimes referred askey
and sometimes asvalue
.
LRU::insert
works too hard.
- It calls
findPage
, followed byaccess
, which in turn calls
findPage
with the same argument, and
deleteKey
, which also traverses the list looking for the same key.
Three traversals with the same argument seems excessive. Consider finding the page which links to the victim.
- It calls
A standard implementation of the linked list may exhibits an undesirable behavior. Continuous deletion and creation of nodes will eventually scatter the nodes all over the heap, leading to the poor referential locality, hence plenty of cache misses during traversal. Since your list by definition has a fixed number of entries, try to keep your
Link
s compactly, say in an array, and reuse them.
LinkList::push_front
shall be streamlined.head = newNode
is executed in both branches; take it out ofif/else
:
newNode->setNext(head);
if (head == nullptr) {
tail = newNode;
}
head = newNode;
Ditto for
push_back
.
I do not endorse trivial getters and setters. There are no invariants to protect, so they just add noise. Making
Link::key
andLink::next
public is just as good.
$endgroup$
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
add a comment |
$begingroup$
One class per file, and each with a dedicated header, is a pretty heavy-handed attempt at getting novices to modularize. As it is often the case, it is also counter-productive in this case.
Link
is an internal implementation-detail ofLinkList
and derived classes, aside from unfortunately and inadvertently being exposed by.delete_at_pos()
.
Fix that one return, and make the class private.
As
Link
is an implementation-detail, there is no sense in trying and failing miserably to hide its details behind a dozen useless public members. Just expose them directly, the only one which might make sense to keep (though that is debatable) is the constructor.Consider a more descriptive name for
LinkList
. MaybeForwardList
?I wonder what
linksAddresses
is for, aside from taking up space.There seems to be a lot of stubbed and half-done code left in
LinkList
. Other code is quite repetitive.I suggest basing an LRU-cache on a
std::vector<T>
or if the items are heavy, astd::vector<std::unique_ptr<T>>
. Also, just a single free function treating such a an LRU-cache might be good enough.There is no reason to explicitly mention
nullptr
all the time for comparisons, this is C++ not Java or C#. Use!
for negation as needed.
$endgroup$
add a comment |
$begingroup$
This is a really useful thing to implement! Good choice of projects. Here are some thoughts on how you could improve it:
Naming
Generally when I think of a linked list I don't think of a list of links. I think of a list of nodes which are linked to each other (via next and/or previous pointers). For that reason, I wouldn't call your class Link
, I'd call it Node
. The link in your class is the next
pointer.
The distinction between push_back()
and insert()
is unclear. In the base class there doesn't appear to be a difference as insert()
simply calls push_back()
. If I make an LRU
object and call push_back()
what are the consequences? (Or push_front()
for that matter?) Perhaps LRU
shouldn't make the methods of LinkList
public. Also, renaming insert()
to add_new_cache_item()
would make clear which a caller should be using given a particular use case.
Related method names should follow the same pattern. You have insert()
but then its opposite is called deleteKey()
. Presumably that's because delete
is a keyword so you couldn't call it that. I'd either rename insert()
to insertKey()
or rename deleteKey()
to simply remove()
.
Do you need setValue
?
Is there a reason why you need a setValue()
method in Link
? It's only called from the constructor. You could simply move the code for it into the constructor and get rid of the public function. (If you added a next
parameter to the constructor you could make the class immutable which is helpful for multithreading, though it would involve changing some of the logic of updating the list.)
Encapsulation
I don't generally like friend
functions. People will say that operator<<()
is always a friend
function. But here you're careful to keep the Link
parameter const
and it only calls the (also const
) getValue()
method. There's nothing here that requires it be a friend
function, so I say get rid of the friend
designation. Just make it a free function in the header.
Double-ended Queue
What you've created actually has a name other than "linked list". It's often called a double-ended queue or deque
for short. They have several useful characteristics such as being O(1) for insertion and deletion at the ends, as you've no doubt discovered. There is a standard container called std::deque
that implements this. (Though I realize you tagged this as "reinventing the wheel", which is always good for learning!)
Avoid raw pointers
I agree with others that you could implement this with a vector
or array
(or deque
) if you wanted to. But whatever you do, you should avoid using raw pointers. They have so many potential pitfalls that they're really not worth it. I recommend using either std::unique_ptr
or std::shared_ptr
depending on the situation. They help avoid a large class of resource management errors.
Symmetry
You should strive to make the interface for your classes symmetrical to make them easier to use and understand. By that I mean that if you have an insert()
method that does one thing, the remove()
or delete()
method should do the opposite. In LRU
you've overridden insert()
to do things like increment the max_len
counter. But then in access()
you have to manually decrement it after calling deleteKey()
. You should also override deleteKey()
to call the base class and decrement max_len
so that someone updating access()
doesn't have to know this fact in the future. They can simply call deleteKey()
and not worry about it.
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f213621%2flru-implementation-in-c-using-a-linkedlist%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
It is misleading that the same field in
Link
is sometimes referred askey
and sometimes asvalue
.
LRU::insert
works too hard.
- It calls
findPage
, followed byaccess
, which in turn calls
findPage
with the same argument, and
deleteKey
, which also traverses the list looking for the same key.
Three traversals with the same argument seems excessive. Consider finding the page which links to the victim.
- It calls
A standard implementation of the linked list may exhibits an undesirable behavior. Continuous deletion and creation of nodes will eventually scatter the nodes all over the heap, leading to the poor referential locality, hence plenty of cache misses during traversal. Since your list by definition has a fixed number of entries, try to keep your
Link
s compactly, say in an array, and reuse them.
LinkList::push_front
shall be streamlined.head = newNode
is executed in both branches; take it out ofif/else
:
newNode->setNext(head);
if (head == nullptr) {
tail = newNode;
}
head = newNode;
Ditto for
push_back
.
I do not endorse trivial getters and setters. There are no invariants to protect, so they just add noise. Making
Link::key
andLink::next
public is just as good.
$endgroup$
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
add a comment |
$begingroup$
It is misleading that the same field in
Link
is sometimes referred askey
and sometimes asvalue
.
LRU::insert
works too hard.
- It calls
findPage
, followed byaccess
, which in turn calls
findPage
with the same argument, and
deleteKey
, which also traverses the list looking for the same key.
Three traversals with the same argument seems excessive. Consider finding the page which links to the victim.
- It calls
A standard implementation of the linked list may exhibits an undesirable behavior. Continuous deletion and creation of nodes will eventually scatter the nodes all over the heap, leading to the poor referential locality, hence plenty of cache misses during traversal. Since your list by definition has a fixed number of entries, try to keep your
Link
s compactly, say in an array, and reuse them.
LinkList::push_front
shall be streamlined.head = newNode
is executed in both branches; take it out ofif/else
:
newNode->setNext(head);
if (head == nullptr) {
tail = newNode;
}
head = newNode;
Ditto for
push_back
.
I do not endorse trivial getters and setters. There are no invariants to protect, so they just add noise. Making
Link::key
andLink::next
public is just as good.
$endgroup$
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
add a comment |
$begingroup$
It is misleading that the same field in
Link
is sometimes referred askey
and sometimes asvalue
.
LRU::insert
works too hard.
- It calls
findPage
, followed byaccess
, which in turn calls
findPage
with the same argument, and
deleteKey
, which also traverses the list looking for the same key.
Three traversals with the same argument seems excessive. Consider finding the page which links to the victim.
- It calls
A standard implementation of the linked list may exhibits an undesirable behavior. Continuous deletion and creation of nodes will eventually scatter the nodes all over the heap, leading to the poor referential locality, hence plenty of cache misses during traversal. Since your list by definition has a fixed number of entries, try to keep your
Link
s compactly, say in an array, and reuse them.
LinkList::push_front
shall be streamlined.head = newNode
is executed in both branches; take it out ofif/else
:
newNode->setNext(head);
if (head == nullptr) {
tail = newNode;
}
head = newNode;
Ditto for
push_back
.
I do not endorse trivial getters and setters. There are no invariants to protect, so they just add noise. Making
Link::key
andLink::next
public is just as good.
$endgroup$
It is misleading that the same field in
Link
is sometimes referred askey
and sometimes asvalue
.
LRU::insert
works too hard.
- It calls
findPage
, followed byaccess
, which in turn calls
findPage
with the same argument, and
deleteKey
, which also traverses the list looking for the same key.
Three traversals with the same argument seems excessive. Consider finding the page which links to the victim.
- It calls
A standard implementation of the linked list may exhibits an undesirable behavior. Continuous deletion and creation of nodes will eventually scatter the nodes all over the heap, leading to the poor referential locality, hence plenty of cache misses during traversal. Since your list by definition has a fixed number of entries, try to keep your
Link
s compactly, say in an array, and reuse them.
LinkList::push_front
shall be streamlined.head = newNode
is executed in both branches; take it out ofif/else
:
newNode->setNext(head);
if (head == nullptr) {
tail = newNode;
}
head = newNode;
Ditto for
push_back
.
I do not endorse trivial getters and setters. There are no invariants to protect, so they just add noise. Making
Link::key
andLink::next
public is just as good.
answered Feb 17 at 0:03
vnpvnp
40.1k232102
40.1k232102
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
add a comment |
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
$begingroup$
For the insert method and access method I found some common code between them so I ended up grouping it into a function. I guess I can change the access method to directly take the pointer and bring it to the end/front of the list. This way the access method need not call findPage. Also for optimizations I am gonna substitute the structures for hashtable + doubly linked list where the hash table will have the (key, memory_location) as an entry. this way we can look up and delte in O(1) time. I just wanted to implement a correctly behaving LRU first, irrespective of the time constraints.
$endgroup$
– piepi
Feb 17 at 0:28
add a comment |
$begingroup$
One class per file, and each with a dedicated header, is a pretty heavy-handed attempt at getting novices to modularize. As it is often the case, it is also counter-productive in this case.
Link
is an internal implementation-detail ofLinkList
and derived classes, aside from unfortunately and inadvertently being exposed by.delete_at_pos()
.
Fix that one return, and make the class private.
As
Link
is an implementation-detail, there is no sense in trying and failing miserably to hide its details behind a dozen useless public members. Just expose them directly, the only one which might make sense to keep (though that is debatable) is the constructor.Consider a more descriptive name for
LinkList
. MaybeForwardList
?I wonder what
linksAddresses
is for, aside from taking up space.There seems to be a lot of stubbed and half-done code left in
LinkList
. Other code is quite repetitive.I suggest basing an LRU-cache on a
std::vector<T>
or if the items are heavy, astd::vector<std::unique_ptr<T>>
. Also, just a single free function treating such a an LRU-cache might be good enough.There is no reason to explicitly mention
nullptr
all the time for comparisons, this is C++ not Java or C#. Use!
for negation as needed.
$endgroup$
add a comment |
$begingroup$
One class per file, and each with a dedicated header, is a pretty heavy-handed attempt at getting novices to modularize. As it is often the case, it is also counter-productive in this case.
Link
is an internal implementation-detail ofLinkList
and derived classes, aside from unfortunately and inadvertently being exposed by.delete_at_pos()
.
Fix that one return, and make the class private.
As
Link
is an implementation-detail, there is no sense in trying and failing miserably to hide its details behind a dozen useless public members. Just expose them directly, the only one which might make sense to keep (though that is debatable) is the constructor.Consider a more descriptive name for
LinkList
. MaybeForwardList
?I wonder what
linksAddresses
is for, aside from taking up space.There seems to be a lot of stubbed and half-done code left in
LinkList
. Other code is quite repetitive.I suggest basing an LRU-cache on a
std::vector<T>
or if the items are heavy, astd::vector<std::unique_ptr<T>>
. Also, just a single free function treating such a an LRU-cache might be good enough.There is no reason to explicitly mention
nullptr
all the time for comparisons, this is C++ not Java or C#. Use!
for negation as needed.
$endgroup$
add a comment |
$begingroup$
One class per file, and each with a dedicated header, is a pretty heavy-handed attempt at getting novices to modularize. As it is often the case, it is also counter-productive in this case.
Link
is an internal implementation-detail ofLinkList
and derived classes, aside from unfortunately and inadvertently being exposed by.delete_at_pos()
.
Fix that one return, and make the class private.
As
Link
is an implementation-detail, there is no sense in trying and failing miserably to hide its details behind a dozen useless public members. Just expose them directly, the only one which might make sense to keep (though that is debatable) is the constructor.Consider a more descriptive name for
LinkList
. MaybeForwardList
?I wonder what
linksAddresses
is for, aside from taking up space.There seems to be a lot of stubbed and half-done code left in
LinkList
. Other code is quite repetitive.I suggest basing an LRU-cache on a
std::vector<T>
or if the items are heavy, astd::vector<std::unique_ptr<T>>
. Also, just a single free function treating such a an LRU-cache might be good enough.There is no reason to explicitly mention
nullptr
all the time for comparisons, this is C++ not Java or C#. Use!
for negation as needed.
$endgroup$
One class per file, and each with a dedicated header, is a pretty heavy-handed attempt at getting novices to modularize. As it is often the case, it is also counter-productive in this case.
Link
is an internal implementation-detail ofLinkList
and derived classes, aside from unfortunately and inadvertently being exposed by.delete_at_pos()
.
Fix that one return, and make the class private.
As
Link
is an implementation-detail, there is no sense in trying and failing miserably to hide its details behind a dozen useless public members. Just expose them directly, the only one which might make sense to keep (though that is debatable) is the constructor.Consider a more descriptive name for
LinkList
. MaybeForwardList
?I wonder what
linksAddresses
is for, aside from taking up space.There seems to be a lot of stubbed and half-done code left in
LinkList
. Other code is quite repetitive.I suggest basing an LRU-cache on a
std::vector<T>
or if the items are heavy, astd::vector<std::unique_ptr<T>>
. Also, just a single free function treating such a an LRU-cache might be good enough.There is no reason to explicitly mention
nullptr
all the time for comparisons, this is C++ not Java or C#. Use!
for negation as needed.
answered Feb 17 at 1:57
DeduplicatorDeduplicator
11.7k1950
11.7k1950
add a comment |
add a comment |
$begingroup$
This is a really useful thing to implement! Good choice of projects. Here are some thoughts on how you could improve it:
Naming
Generally when I think of a linked list I don't think of a list of links. I think of a list of nodes which are linked to each other (via next and/or previous pointers). For that reason, I wouldn't call your class Link
, I'd call it Node
. The link in your class is the next
pointer.
The distinction between push_back()
and insert()
is unclear. In the base class there doesn't appear to be a difference as insert()
simply calls push_back()
. If I make an LRU
object and call push_back()
what are the consequences? (Or push_front()
for that matter?) Perhaps LRU
shouldn't make the methods of LinkList
public. Also, renaming insert()
to add_new_cache_item()
would make clear which a caller should be using given a particular use case.
Related method names should follow the same pattern. You have insert()
but then its opposite is called deleteKey()
. Presumably that's because delete
is a keyword so you couldn't call it that. I'd either rename insert()
to insertKey()
or rename deleteKey()
to simply remove()
.
Do you need setValue
?
Is there a reason why you need a setValue()
method in Link
? It's only called from the constructor. You could simply move the code for it into the constructor and get rid of the public function. (If you added a next
parameter to the constructor you could make the class immutable which is helpful for multithreading, though it would involve changing some of the logic of updating the list.)
Encapsulation
I don't generally like friend
functions. People will say that operator<<()
is always a friend
function. But here you're careful to keep the Link
parameter const
and it only calls the (also const
) getValue()
method. There's nothing here that requires it be a friend
function, so I say get rid of the friend
designation. Just make it a free function in the header.
Double-ended Queue
What you've created actually has a name other than "linked list". It's often called a double-ended queue or deque
for short. They have several useful characteristics such as being O(1) for insertion and deletion at the ends, as you've no doubt discovered. There is a standard container called std::deque
that implements this. (Though I realize you tagged this as "reinventing the wheel", which is always good for learning!)
Avoid raw pointers
I agree with others that you could implement this with a vector
or array
(or deque
) if you wanted to. But whatever you do, you should avoid using raw pointers. They have so many potential pitfalls that they're really not worth it. I recommend using either std::unique_ptr
or std::shared_ptr
depending on the situation. They help avoid a large class of resource management errors.
Symmetry
You should strive to make the interface for your classes symmetrical to make them easier to use and understand. By that I mean that if you have an insert()
method that does one thing, the remove()
or delete()
method should do the opposite. In LRU
you've overridden insert()
to do things like increment the max_len
counter. But then in access()
you have to manually decrement it after calling deleteKey()
. You should also override deleteKey()
to call the base class and decrement max_len
so that someone updating access()
doesn't have to know this fact in the future. They can simply call deleteKey()
and not worry about it.
$endgroup$
add a comment |
$begingroup$
This is a really useful thing to implement! Good choice of projects. Here are some thoughts on how you could improve it:
Naming
Generally when I think of a linked list I don't think of a list of links. I think of a list of nodes which are linked to each other (via next and/or previous pointers). For that reason, I wouldn't call your class Link
, I'd call it Node
. The link in your class is the next
pointer.
The distinction between push_back()
and insert()
is unclear. In the base class there doesn't appear to be a difference as insert()
simply calls push_back()
. If I make an LRU
object and call push_back()
what are the consequences? (Or push_front()
for that matter?) Perhaps LRU
shouldn't make the methods of LinkList
public. Also, renaming insert()
to add_new_cache_item()
would make clear which a caller should be using given a particular use case.
Related method names should follow the same pattern. You have insert()
but then its opposite is called deleteKey()
. Presumably that's because delete
is a keyword so you couldn't call it that. I'd either rename insert()
to insertKey()
or rename deleteKey()
to simply remove()
.
Do you need setValue
?
Is there a reason why you need a setValue()
method in Link
? It's only called from the constructor. You could simply move the code for it into the constructor and get rid of the public function. (If you added a next
parameter to the constructor you could make the class immutable which is helpful for multithreading, though it would involve changing some of the logic of updating the list.)
Encapsulation
I don't generally like friend
functions. People will say that operator<<()
is always a friend
function. But here you're careful to keep the Link
parameter const
and it only calls the (also const
) getValue()
method. There's nothing here that requires it be a friend
function, so I say get rid of the friend
designation. Just make it a free function in the header.
Double-ended Queue
What you've created actually has a name other than "linked list". It's often called a double-ended queue or deque
for short. They have several useful characteristics such as being O(1) for insertion and deletion at the ends, as you've no doubt discovered. There is a standard container called std::deque
that implements this. (Though I realize you tagged this as "reinventing the wheel", which is always good for learning!)
Avoid raw pointers
I agree with others that you could implement this with a vector
or array
(or deque
) if you wanted to. But whatever you do, you should avoid using raw pointers. They have so many potential pitfalls that they're really not worth it. I recommend using either std::unique_ptr
or std::shared_ptr
depending on the situation. They help avoid a large class of resource management errors.
Symmetry
You should strive to make the interface for your classes symmetrical to make them easier to use and understand. By that I mean that if you have an insert()
method that does one thing, the remove()
or delete()
method should do the opposite. In LRU
you've overridden insert()
to do things like increment the max_len
counter. But then in access()
you have to manually decrement it after calling deleteKey()
. You should also override deleteKey()
to call the base class and decrement max_len
so that someone updating access()
doesn't have to know this fact in the future. They can simply call deleteKey()
and not worry about it.
$endgroup$
add a comment |
$begingroup$
This is a really useful thing to implement! Good choice of projects. Here are some thoughts on how you could improve it:
Naming
Generally when I think of a linked list I don't think of a list of links. I think of a list of nodes which are linked to each other (via next and/or previous pointers). For that reason, I wouldn't call your class Link
, I'd call it Node
. The link in your class is the next
pointer.
The distinction between push_back()
and insert()
is unclear. In the base class there doesn't appear to be a difference as insert()
simply calls push_back()
. If I make an LRU
object and call push_back()
what are the consequences? (Or push_front()
for that matter?) Perhaps LRU
shouldn't make the methods of LinkList
public. Also, renaming insert()
to add_new_cache_item()
would make clear which a caller should be using given a particular use case.
Related method names should follow the same pattern. You have insert()
but then its opposite is called deleteKey()
. Presumably that's because delete
is a keyword so you couldn't call it that. I'd either rename insert()
to insertKey()
or rename deleteKey()
to simply remove()
.
Do you need setValue
?
Is there a reason why you need a setValue()
method in Link
? It's only called from the constructor. You could simply move the code for it into the constructor and get rid of the public function. (If you added a next
parameter to the constructor you could make the class immutable which is helpful for multithreading, though it would involve changing some of the logic of updating the list.)
Encapsulation
I don't generally like friend
functions. People will say that operator<<()
is always a friend
function. But here you're careful to keep the Link
parameter const
and it only calls the (also const
) getValue()
method. There's nothing here that requires it be a friend
function, so I say get rid of the friend
designation. Just make it a free function in the header.
Double-ended Queue
What you've created actually has a name other than "linked list". It's often called a double-ended queue or deque
for short. They have several useful characteristics such as being O(1) for insertion and deletion at the ends, as you've no doubt discovered. There is a standard container called std::deque
that implements this. (Though I realize you tagged this as "reinventing the wheel", which is always good for learning!)
Avoid raw pointers
I agree with others that you could implement this with a vector
or array
(or deque
) if you wanted to. But whatever you do, you should avoid using raw pointers. They have so many potential pitfalls that they're really not worth it. I recommend using either std::unique_ptr
or std::shared_ptr
depending on the situation. They help avoid a large class of resource management errors.
Symmetry
You should strive to make the interface for your classes symmetrical to make them easier to use and understand. By that I mean that if you have an insert()
method that does one thing, the remove()
or delete()
method should do the opposite. In LRU
you've overridden insert()
to do things like increment the max_len
counter. But then in access()
you have to manually decrement it after calling deleteKey()
. You should also override deleteKey()
to call the base class and decrement max_len
so that someone updating access()
doesn't have to know this fact in the future. They can simply call deleteKey()
and not worry about it.
$endgroup$
This is a really useful thing to implement! Good choice of projects. Here are some thoughts on how you could improve it:
Naming
Generally when I think of a linked list I don't think of a list of links. I think of a list of nodes which are linked to each other (via next and/or previous pointers). For that reason, I wouldn't call your class Link
, I'd call it Node
. The link in your class is the next
pointer.
The distinction between push_back()
and insert()
is unclear. In the base class there doesn't appear to be a difference as insert()
simply calls push_back()
. If I make an LRU
object and call push_back()
what are the consequences? (Or push_front()
for that matter?) Perhaps LRU
shouldn't make the methods of LinkList
public. Also, renaming insert()
to add_new_cache_item()
would make clear which a caller should be using given a particular use case.
Related method names should follow the same pattern. You have insert()
but then its opposite is called deleteKey()
. Presumably that's because delete
is a keyword so you couldn't call it that. I'd either rename insert()
to insertKey()
or rename deleteKey()
to simply remove()
.
Do you need setValue
?
Is there a reason why you need a setValue()
method in Link
? It's only called from the constructor. You could simply move the code for it into the constructor and get rid of the public function. (If you added a next
parameter to the constructor you could make the class immutable which is helpful for multithreading, though it would involve changing some of the logic of updating the list.)
Encapsulation
I don't generally like friend
functions. People will say that operator<<()
is always a friend
function. But here you're careful to keep the Link
parameter const
and it only calls the (also const
) getValue()
method. There's nothing here that requires it be a friend
function, so I say get rid of the friend
designation. Just make it a free function in the header.
Double-ended Queue
What you've created actually has a name other than "linked list". It's often called a double-ended queue or deque
for short. They have several useful characteristics such as being O(1) for insertion and deletion at the ends, as you've no doubt discovered. There is a standard container called std::deque
that implements this. (Though I realize you tagged this as "reinventing the wheel", which is always good for learning!)
Avoid raw pointers
I agree with others that you could implement this with a vector
or array
(or deque
) if you wanted to. But whatever you do, you should avoid using raw pointers. They have so many potential pitfalls that they're really not worth it. I recommend using either std::unique_ptr
or std::shared_ptr
depending on the situation. They help avoid a large class of resource management errors.
Symmetry
You should strive to make the interface for your classes symmetrical to make them easier to use and understand. By that I mean that if you have an insert()
method that does one thing, the remove()
or delete()
method should do the opposite. In LRU
you've overridden insert()
to do things like increment the max_len
counter. But then in access()
you have to manually decrement it after calling deleteKey()
. You should also override deleteKey()
to call the base class and decrement max_len
so that someone updating access()
doesn't have to know this fact in the future. They can simply call deleteKey()
and not worry about it.
answered Feb 17 at 6:25
user1118321user1118321
10.9k11145
10.9k11145
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f213621%2flru-implementation-in-c-using-a-linkedlist%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown