Hashtable Copy Constructor Not working

[xerofoify]

Either I am doing something very stupid or this is working and the tester I am using is wrong:
template <class TYPE>
HashTable<TYPE>::HashTable(const HashTable<TYPE>& other) {
table_ = new Record*[other.cap];
size_ = other.size_;
hash = other.hash;
cap = other.cap;
std::cout << cap << size_ << "\n";
for (int i = 0; i<other.cap_; i++) {
if (table_[i] == nullptr) {
continue;
}
update(other.table_[i]->key_,other.table_[i]->data_);
}
}
//Updates record with the string passed to the new data for the record's data element and returns true if found/updated otherwise false
template <class TYPE>
bool HashTable<TYPE>::update(const string& key, const TYPE& value) {
int hashvalue = hash(key) % cap;
Record* prev = nullptr;
Record* entry = table_[hashvalue];
while (entry != nullptr && entry->getKey() != key) {
prev = entry;
entry = entry->getNext();
}

if (entry == nullptr) {
entry = new Record(key, value);
if (prev == nullptr) {
// insert as first bucket
table_[hashvalue] = entry;
}
else {
prev->setNext(entry);
}
}
else {
// just update the value
entry->setData(value);
}
size_++;
return true;
}

[Ian Collins]

On 12/28/16 05:31 PM, xerofoify wrote:
> Either I am doing something very stupid or this is working and the tester I am using is wrong:

In neither of your postings did you define "not working".

--
Ian

[xerofoify]

It does not pass the tester as one idea is not there after coping it over. I can't see how as update itself works but not in the for loop.

[xerofoify]

On Tuesday, December 27, 2016 at 11:36:52 PM UTC-5, Ian Collins wrote:

Basically the tester is not finding one pair correctly even through update passes its test. I am wondering how the for loop does not work as update itself clearly does.

[Jens Thoms Toerring]

xerofoify <xero...@gmail.com> wrote:
> Either I am doing something very stupid or this is working and the tester
> I am using is wrong:

Sounds a bit as if the "tester" (whatever that might be)
is the compiler, at least if the code you posted is for
real;-)

> template <class TYPE>
> HashTable<TYPE>::HashTable(const HashTable<TYPE>& other) {
> table_ = new Record*[other.cap];

Now you've got an array of 'other.cap' uninitialized pointers.
to me it would look rather useful to set them all to 'nullptr'
to make it sure that none of them points to any random
memory location yet. You could achive that by using

table_ = new Record* [other.cap] ();

> size_ = other.size_;

I'm not sure, but I'd guess that better should be ini-
tialized to 0, at least in case my guess is correct that
this should contain the number of entries in the hash.
But see my comments below.

> hash = other.hash;
> cap = other.cap;

All of 'table_', 'size_', 'cap' and 'hash' obviousy must be
member variables. One convention is to have their names end
in an underscore. But you only use that for some of them.
Why?

> std::cout << cap << size_ << "\n";
> for (int i = 0; i<other.cap_; i++) {

And here there suddenly seems to be another member variable,
named 'cap_'. Is that a typo or do you really have two
member variables, one named 'cap' and one named 'cap_'?
If that's the case why and what is 'cap_' then not also
initialized in the new instance?

> if (table_[i] == nullptr) {

Here you test for random values that the 'table_' array
contains after its allocation above. My guess is that
you should test for 'other.table_[i]' instead.

> continue;
> }
> update(other.table_[i]->key_,other.table_[i]->data_);

Here comes the next question: as far as one can surmise from
the update() function a 'Record' seems to a linked list of
key-value pairs. So you'll need to iterate over that instead
of copying just the (probably) first element of that linked
list. Something along the lines of

Record * cur_rec = other.table_[i];
while (cur_rec) {
update(cur_rec->key_, cur_rec->data_);
cur_rec = cur_rec->getNext();

}

> }

> //Updates record with the string passed to the new data for the record's data element and returns true if found/updated otherwise false

This comment definitely does not describe what the function is
doing (and the name is a bit of a mis-nomer since it will not
only update existing elements but also add new ones). It never
does return 'false' but always 'true', so it having a return
value doesn't make too much sense.

> template <class TYPE>
> bool HashTable<TYPE>::update(const string& key, const TYPE& value) {
> int hashvalue = hash(key) % cap;
> Record* prev = nullptr;
> Record* entry = table_[hashvalue];
> while (entry != nullptr && entry->getKey() != key) {
> prev = entry;
> entry = entry->getNext();
> }

> if (entry == nullptr) {
> entry = new Record(key, value);
> if (prev == nullptr) {
> // insert as first bucket
> table_[hashvalue] = entry;
> }
> else {
> prev->setNext(entry);
> }
> }
> else {
> // just update the value
> entry->setData(value);
> }
> size_++;

This looks strange. The way 'size_' is used here indiceates
that it counts how often the entries were modified (added or
updated). But the name would make more sense if it would
count the total number of entries. In that case it should
only be incremented when a new entry was added, not also
when one was merely changed. (But in that case you'd have
to set it to 0, not copy it from the other table in your
copy constructor above!0

> return true;
> }

Here's a somewhat condensed form of how I guess your copy
constuctor might look like in the end:

template <class TYPE>
HashTable<TYPE>::HashTable(const HashTable<TYPE>& other)

: table_(new Record* [other.cap] ()),
: size_(0),
: hash(other.hash),
: cap(other.cap)
{
for (size_t i = 0; i < other.cap; ++i) {
Record * cur_rec = other.table_[i];
while (cur_rec) {
update(cur_rec->getKey(), cur_rec->getValue());
cur_rec = cur_rec->getNext();
}
}
}
Regards, Jens
--
\ Jens Thoms Toerring ___ j...@toerring.de
\__________________________ http://toerring.de