Skip to content

Comments

added students solution to review#1

Open
JJJazl wants to merge 1 commit intomasterfrom
add-solution-to-review
Open

added students solution to review#1
JJJazl wants to merge 1 commit intomasterfrom
add-solution-to-review

Conversation

@JJJazl
Copy link
Owner

@JJJazl JJJazl commented Sep 17, 2024

No description provided.

@@ -1,48 +1,170 @@
package core.basesyntax;

//import jdk.internal.util.ArraysSupport;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Please don't add redundant imports and commented lines to your code

Comment on lines +9 to +10


Copy link
Owner Author

Choose a reason for hiding this comment

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

Please don’t add redundant empty lines to your code

Comment on lines +11 to +12
private static final int def_capacity = 10;
private static final Object[] empty_arraydata = {};
Copy link
Owner Author

Choose a reason for hiding this comment

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

  1. Pay attention to constants naming convention
    https://mate-academy.github.io/style-guides/java/java.html#s5.2.4-constant-names
  2. Use a full name for variables to make code more clear. For example:
private static final int DEFAULT_CAPACITY = 10;

Comment on lines +13 to +14


Copy link
Owner Author

Choose a reason for hiding this comment

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

Please don’t add redundant empty lines to your code

private static final Object[] empty_arraydata = {};


private T[] array = (T[]) new Object[def_capacity];
Copy link
Owner Author

Choose a reason for hiding this comment

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

  1. Use informative names for your variables. For example, elementData provides more insight than an array
  2. Redundant typecast to generic array. Generic type T will be erased to Object after compilation, so you can directly use a Object[]
    Example below:
    private Object[] elementData;
  3. It might be better to have 2 constructors in order to initialize ArrayList with default and specified initial capacity, but maybe it's not a required for this task, so I will specify it to be sure. Anyway, you can see how it would look like below:
public ArrayList() {
        this.elementData = EMPTY_ARRAYDATA;
    }

    public ArrayList(int initialCapacity) {
        if (initialCapacity < 0) {
            throw new IllegalArgumentException("Initial capacity must be great than 0");
        }
        this.elementData = new Object[initialCapacity];
    }

}


public static Object[] listToArray(List<?> list) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

  1. This method is not a part of public API, so make it private and non-static
  2. Use List with generic type T instead of raw type, because we want to be sure we stick to the same type
    P.S. you can use @SuppressWarnings("unchecked") to avoid "Unchecked cast: 'java.lang.Object[]' to 'T[]'" warning
    Example below:
@SuppressWarnings("unchecked")
public T[] listToArray(List<T> list) {
}

}
return tempArray;
}
return null;
Copy link
Owner Author

Choose a reason for hiding this comment

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

It can cause a NPE, so return an EMPTY_ARRAYDATA instead

Comment on lines +161 to +168
boolean empty = true;
for (T element : array) {
if (element != null) {
empty = false;
break;
}
}
return empty;
Copy link
Owner Author

Choose a reason for hiding this comment

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

You can just check if size == 0. It will be O(1) instead of O(n) time complexity

Comment on lines +130 to +149
found:
{
if (element == null) {
for (; index < size; index++) {
if (array[index] == null) {
remove(index);
break found;
}
}
} else {
for (; index < size; index++) {
if (element.equals(array[index])) {
remove(index);
break found;
}
}
throw new NoSuchElementException();
}
}
return element;
Copy link
Owner Author

Choose a reason for hiding this comment

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

  1. It seems like copy of the original ArrayList using the found label. This is generally considered as a bad practice, because it makes code more verbose and harder to read, so I suggest rewrite it simply using for statements.
  2. Make your exceptions informative
    https://mate-academy.github.io/jv-program-common-mistakes/java-core/collections/array-list.html#make-your-exceptions-informative

import java.util.Arrays;
import java.util.NoSuchElementException;


Copy link
Owner Author

Choose a reason for hiding this comment

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

Please don’t add redundant empty lines to your code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants