Is my array class implementation ok?

Hi guys, I’m Ceci, I’m trying to solve the data structures and exercises from that course on JavaScript.
This is my implementation.
Please provide some feedback on how to improve the solution or if you have another solution in JavaScript share with me. :slight_smile:

class MyArrayClass {
  #length;
  #data;

  constructor(length = 0) {  
    this.#length = length; 
    this.#data = []; 
  }

  isFull() {
    let newItems = [];
    if(this.#data.length === this.#length) {
      newItems = [this.#length*2];
    }

    for(let i = 0; i < this.#length; i++) {
      newItems[i] = this.#data[i];
    }

    return this.#data = newItems;
  }

  push(item) {
    this.isFull();
    this.#data[this.#length] = item;
    this.#length++;
    return this.#data;
  }

  pop() {
    delete this.#data[this.#length - 1];
    this.#length--;
    return this.#data;
  }

  validIndex(index) {
    if (index < 0 || index >= this.#length) {
      throw new Error("Invalid argument excpetion - BANG!!");
    }
  }

  removeAt(index) {
    this.validIndex(index);

    for (let i = index; i < this.#length; i++) {
      this.#data[i] = this.#data[i + 1];
    }

    delete this.#data[this.#length - 1]; 
    this.#length--;
    return this.#data;
  }

  indexOf(item) {
    for(let i = 0; i < this.#length; i++) {
      if(this.#data[i] === item) {
        return i;
      }
    }

    return -1;
  }

  max() {
    let max = this.#data[0];

    for(let i = 0;  i < this.#length; i++) {
      if(max < this.#data[i])
       max = this.#data[i];
    }

    return max;
  }

  intersect(other) {
    const intersectedArray = this.#data.filter(value => other.includes(value));
    return intersectedArray;
  }

  reverse() {
    let newItems = new Array;
    for (let i = 0; i < this.#length; i++) {
      newItems[i] = this.#data[this.#length - i - 1];
    }

    this.#data = newItems;
    return newItems;
  }

  insertAt(item, index) {
    this.validIndex(index);

    for (let i = this.#length; i >= index; i--) {
      this.#data[i] = this.#data[i - 1];
    }

    this.#data[index] = item;
    this.#length++;
    return this.#data;
  }
}

Did you test your implementation against various inputs and edge cases? There are several problems in your code ranging from minor to severe. I only took a glance at it, so there might be more. But here’s what I’ve found.

Initializing an array with length larger than 0 breaks array. Try creating an array of length 1 and push an element to it. See what happens. You need to have a capacity as well as length.

This code doesn’t do what you are trying to do. All it does is create a new array with one element that is length * 2

I wouldn’t call it isFull because the name implies it will return a boolean value without side effects. It is just a conventional thing.

If you decided to use private feature, I would apply it consistently. For example, why is validIndex() and isFull() not private?

In your implementation, how are users supposed to know how many elements are in the array?

max() doesn’t really belong to an array. What happens when array contains non-numeric values?
Also, if max should be there why not min and other aggregating functions? You can see where I’m going.

The message should indicate index out of range.

Usability-wise, pop() should return the last element and not the array. Otherwise, how would the caller access the popped value?

Lastly, there are many places where you return a Javascript array type. If your goal is implementing an array, your array class should return your array.

1 Like

Did you follow what Mosh asked in this exercise? I’m trying to do that but with JavaScript that’s it, just the methods he asked and what he said they should do. the pop will be the remove in this case the min he did not asked and the error message yeah I could be more explicit but I’m aking help in the implementation of the code, to the people who followed Mosh tutorial, the only thing I find usefull about your reply is that validIndex( ) and isFull( ) should be private.

Nope, I didn’t take that course. I’m just talking about the array implementation in general.

So, is this a correct behavior of the array from Mosh’s course?

let a = new MyArrayClass(1);

a.push('foo'); // returns [undefined, 'foo']

Also, this code doesn’t expand your array by double. If so, why put it at all? Just to artificially demonstrate the concept of expanding the array?

if(this.#data.length === this.#length) {
      newItems = [this.#length*2];
}

If you are only seeking for feedbacks from people who took Mosh’s data structure and what’s required in the exercise explicitly. I have nothing more to say.

is this better now?

class MyArrayClass {
  #length;
  #data;

  constructor(length = 0) {   
    this.#length = length;
    this.#data = new Array(this.#length); 
  }

  #resizeIfRequired() {
    let newItems = [];
    if(this.#data.length === this.#length) {
      newItems = [this.#length*2];
    }

    for(let i = 0; i < this.#length; i++) {
      newItems[i] = this.#data[i];
    }

    this.#data = newItems;
  }

  #validIndex(index) {
    if (index < 0 || index >= this.#length) {
      throw new Error("Index out of range");
    }
  }

  insert(item) {
    this.#resizeIfRequired();
    this.#data[this.#length] = item;
    this.#length++;

    return item;
  }

  remove() {
    delete this.#data[this.#length - 1];
    this.#length--;
    return this.#data;
  }

  removeAt(index) {
    this.#validIndex(index);

    for (let i = index; i < this.#length; i++) {
      this.#data[i] = this.#data[i + 1];
    }

    delete this.#data[this.#length - 1]; 
    this.#length--;
    return this.#data;
  }

  indexOf(item) {
    for(let i = 0; i < this.#length; i++) {
      if(this.#data[i] === item) {
        return i;
      }
    }

    return -1;
  }

  max() {
    let max = this.#data[0];

    for(let i = 0;  i < this.#length; i++) {
      if(max < this.#data[i])
       max = this.#data[i];
    }

    return max;
  }

  intersect(other) {
    const intersectedArray = this.#data.filter(value => other.includes(value));
    return intersectedArray;
  }

  reverse() {
    let newItems = [];
    for (let i = 0; i < this.#length; i++) {
      newItems[i] = this.#data[this.#length - i - 1];
    }

    this.#data = newItems;
    return newItems;
  }

  insertAt(item, index) {
    this.#validIndex(index);
    this.#resizeIfRequired();

    for (let i = this.#length - 1; i >= index; i--) {
      this.#data[i + 1] = this.#data[i];
    }

    this.#data[index] = item;
    this.#length++;
    return this.#data;
  }

  print() {
    for(let i = 0; i < this.#length; i++) {
      console.log(this.#data[i]);
    }
  }
}

const arrayInstance = new MyArrayClass();
arrayInstance.insert(3);
arrayInstance.insert(1);
arrayInstance.insert(5);


const max = arrayInstance.max();
const intersect = arrayInstance.intersect([1, 0, 6, 11, 5]);
const reverse = arrayInstance.reverse(); 
const positionToInsert = arrayInstance.insertAt(11, 0);
arrayInstance.print();
console.log("----------------------");

console.log(max);
console.log(intersect);
console.log(reverse);

can you share your solution?

Unfortunately, it is mostly the same in terms of functionality other than some naming changes. Initializing the array with length > 0 is still broken.

It is difficult to share my solution because I don’t know what is required by the exercise since I don’t have access to the course.

But I can share a bare minimum implementation.

class ArrayList {
  constructor(capacity=100) {
    if (capacity < 1) {
      throw Error('Invalid argument: capacity must be at least 1')
    }

    // Suppose this is a fixed size array
    // This is demo purpose only, don't do this in Javascript.
    this._values = Array.from(Array(capacity));
    this._capacity = capacity;
    this.length = 0;
  }

  push(value) {
    this._resize();
    this._values[this.length] = value;
    this.length += 1;
  }

  pop(value) {
    if (this.length === 0) {
      throw Error('Not enough element to pop')
    }
    const removed = this._values[this.length - 1];
    this._values[this.length - 1] = undefined;
    this.length -= 1;
    return removed;
  }

  // Simulates a[i]
  get(index) {
    this._checkIndexOutOfBound(index);
    return this._values[index];
  }

  // Simulates a[i] = value
  set(index, value) {
    this._checkIndexOutOfBound(index);
    this._values[index] = value;
  }

  removeAt(index) {
    this._checkIndexOutOfBound(index);
    for(let i = index; i < this.length; ++i) {
      this._values[i] = this._values[i + 1];
    }
    this.length -= 1;
  }

  _checkIndexOutOfBound(index) {
    if (index < 0 || index >= this.length) {
      throw Error('Index of out bound');
    }
  }

  _resize() {
    if (this.length >= this._capacity) {
      const newCapacity = this._capacity * 2;
      // This only simulates expanding the array list. It is meaningless otherwise.
      const copy = Array.from(Array(newCapacity));
      for (let i = 0; i < this._capacity; ++i) {
        copy[i] = this._values[i];
      }

      this._capacity = newCapacity;
      this._values = copy;
    }
  }
}

And some dirty tests but does the job.

let a = new ArrayList(1);
// Test get
try {
  a.get(0);
  console.log('Get throws index out of bound', false);
} catch(error) {
  console.log('Get throws index out of bound', true);
}

try {
  a.get(-1);
  console.log('Get throws index out of bound', false);
} catch(error) {
  console.log('Get throws index out of bound', true);
}

try {
  a.get(1);
  console.log('Get throws index out of bound', false);
} catch(error) {
  console.log('Get throws index out of bound', true);
}

// Test push
a.push(1);
console.log('Push 1', a.get(0) === 1, a.length === 1, a._capacity === 1);
a.push(2);
console.log('Push 2', a.get(1) === 2, a.length === 2, a._capacity === 2);
a.push(3);
console.log('Push does resizes', a.get(0) === 1, a.get(1) === 2, a.get(2) === 3, a.length === 3, a._capacity === 4);

// Test pop
console.log('Pop', a.pop() === 3, a.length === 2, a._capacity === 4);
a.pop();
a.pop();
try {
  a.pop()
  console.log('Pop empty array throws', false);
} catch(error) {
  console.log('Pop empty array throws', true);
}


// Test removeAt
a = new ArrayList(1);
a.push(1);
a.removeAt(0);
console.log('RemoveAt first element works', a.length === 0);
try {
  a.removeAt(0);
  console.log('RemoveAt throws index out of bound', false);
} catch(error) {
  console.log('RemoveAt throws index out of bound', true);
}

a = new ArrayList(1);
a.push(1);
a.push(2);
a.removeAt(1);
console.log('RemoveAt last element works', a.get(0) === 1, a.length === 1);

a = new ArrayList(1);
a.push(1);
a.push(2);
a.push(3);
a.removeAt(1);
console.log('RemoveAt middle element works', a.get(0) === 1, a.get(1) === 3, a.length === 2);

// Test set
a = new ArrayList(3);
try {
  a.set(1, 'foo');
  console.log('Set throws index of range', false);
} catch(error) {
  console.log('Set throws index of range', true);
}
a.push(1);
a.set(0, 'foo');
console.log('Set replaces existing element', a.get(0) === 'foo')


// Test loop
const expected = [1,2,3];
a = new ArrayList();
a.push(1);
a.push(2);
a.push(3);

try {
  for(let i = 0; i < a.length; ++i) {
    if (expected[i] !== a.get(i)) {
      throw Error()
    }
  }
  console.log('Can be iterated over', true);
} catch(error) {
  console.log('Can be iterated over', false);
}

Anyway, what I’m pointing about your implementation is this:

  1. You treat capacity and length to be the same thing.
  2. Your array doesn’t support indexing. How do you access 3rd element?
  3. Your array can’t be iterated over easily because you don’t expose length.

You have to think about how your array will be used 90% of the time.

2 Likes

thanks, I didn’t know capacity and length where different, what is the difference exactly?

Capacity is how many elements you can store in your current array before resizing.
Length or size is the actual number of elements in the array.

If you have a cup that holds 255ml that’s a cup with 255ml capacity. If in that cup, you have 100ml of coffee, then 100ml is the size.

You need both capacity and size in a dynamically resizing array because size can grow or shrink within capacity.

2 Likes

thanks a lot random coder, I think the “_” before every variable is convention for private declaration? thanks a lot this forum is very supportive. :smiley:

As of recently, # is the official private keyword in Javascript. Underscore is an old convention used in many dynamic languages. If you must use private fields for some reason in Javascript, # is the way to go because you can actually hide those.

Normally, I wouldn’t bother making something private for the exercise code but I used it anyway in this case.

2 Likes

This is my final version, I used all your code.

/*
- Create an array class that works similar to the JavaScript Array class itself.
- Add a capacity of 100.
- Methods: push, pop, get, set, getList, removeAt, indexOf.
- private methods: resize, checkIndexOutOfBounds.
- Bonus: Max, intersect, reverse, insertAt. 
*/

class ArrayList {
  #items;
  #capacity;

  constructor(capacity = 100) {
    if(capacity < 0)
      throw Error("Invalid argument: capacity should be at least 1");

    this.#items = Array.from(Array(capacity));
    this.#capacity = capacity;
    this.length = 0;

    if(capacity === 0)
      this.#items = [];
  }

  #resize() {
    if(this.#capacity < this.length) {
      let newCapacity = this.#capacity*2;
      let copy = Array.from(Array(newCapacity));

      for(let i = 0; i < this.#capacity; i++)
        copy[i] = this.#items[i];

      this.#capacity = newCapacity;
      this.#items = copy;
    }
  }

  #checkIndexOutOfBounds(index) {
    if(index < 0 || index >= this.length)
      throw Error("Index out of bounds");
  }

  push(item) {
    this.#resize();
    this.#items[this.length] = item;
    this.length++;
  }

  pop() {
    if(this.length === 0)
      throw Error("Not enough element to pop");

    const removed = this.#items[this.length - 1];
    this.#items[this.length - 1] = undefined;
    this.length--; 
    return removed;
  }

  get(index) {
    this.#checkIndexOutOfBounds(index);
    return this.#items[index];
  }

  set(item, index) {
    this.#checkIndexOutOfBounds(index);
    this.#items[index] = item;
  }

  getList() {
    return this.#items;
  }

  removeAt(index) {
    this.#checkIndexOutOfBounds(index);
    for(let i = index; i < this.#capacity; i++)
      this.#items[i] = this.#items[i + 1];

    this.length--;
  }

  indexOf(item) {
    for(let i = 0; i < this.length; i++)
      if(this.#items[i] === item) 
        return i;

    return -1;
  }

  max() {
    let item = 0;
    for(let i = 0; i < this.length; i++)
      if(this.#items[i] > item)
        item = this.#items[i];

    return item;
  }

  intersect(other) {
    return this.#items.filter(item => other.includes(item));
  }

  reverse() {
    let newItems = new Array(this.#capacity);
    for(let i = 0; i < this.#capacity; i++) 
      newItems[i] = this.#items[this.#capacity - i - 1];
    
    this.#items = newItems;
    return newItems;
  }

  insertAt(item, index) {
    this.#checkIndexOutOfBounds(index);
    this.#resize();

    for(let i = this.#capacity - 1; i >= index; i--)
      this.#items[i + 1] = this.#items[i];

    this.#items[index] = item;
    this.length++;
  }
}