Create 20251204.java

[argondev22]

Status

Accepted

Runtime

41 ms

Order

O(n)

Memory

47.25 MB

Order

O(1)

Time Taken

m s

[Copilot]

いくつか問題点があります。順番に説明しますね。

主な問題点

1. HashMapの型定義が間違っている

Map<String, Integer> hashMap = new HashMap<>();

キーをString型にしていますが、実際に格納しているのはchar型です。これは型の不一致でコンパイルエラーになるはずです。

修正:

Map<Character, Integer> hashMap = new HashMap<>();

2. edge caseの条件が間違っている

if (s.length() <= 1) {
    return -1;
}

文字列の長さが1の場合、その1文字はユニークなので、インデックス0を返すべきです。長さ0の場合のみ-1を返します。

修正:

if (s.length() == 0) {
    return -1;
}
// または、この条件自体を削除しても動作します

3. カウント処理が冗長

if (hashMap.containsKey(e)) {
    Integer tmp = hashMap.get(e);
    tmp++;
    hashMap.put(e, tmp);
} else {
    hashMap.put(e, 1);
}

この処理はgetOrDefault()やmerge()を使うともっと簡潔に書けます。

改善案:

hashMap.put(e, hashMap.getOrDefault(e, 0) + 1);

修正版コード

class Solution {
    public int firstUniqChar(String s) {
        // edge case
        if (s.length() == 0) {
            return -1;
        }

        Map<Character, Integer> hashMap = new HashMap<>();

        // 各文字の出現回数をカウント
        for (char e : s.toCharArray()) {
            hashMap.put(e, hashMap.getOrDefault(e, 0) + 1);
        }

        // 最初に出現するユニークな文字を探す
        for (int i = 0; i < s.length(); i++) {
            char e = s.charAt(i);
            if (hashMap.get(e) == 1) {
                return i;
            }
        }

        return -1;
    }
}

さらなる改善案

edge caseの処理は実は不要です。空文字列でも正しく動作します:

class Solution {
    public int firstUniqChar(String s) {
        Map<Character, Integer> hashMap = new HashMap<>();

        for (char e : s.toCharArray()) {
            hashMap.put(e, hashMap.getOrDefault(e, 0) + 1);
        }

        for (int i = 0; i < s.length(); i++) {
            if (hashMap.get(s.charAt(i)) == 1) {
                return i;
            }
        }

        return -1;
    }
}

この実装で、すべてのケースが正しく処理されます。

[Copilot]

変数名が不明瞭（-0.5点）

問題点： 変数名eは短すぎて意味が不明確です。より説明的な名前を使用すべきです。

現在のコード：

for (char e : s.toCharArray()) {

改善案：

for (char c : s.toCharArray()) {

または

for (char ch : s.toCharArray()) {

文字を表す変数にはc、ch、またはcharacterなどの一般的な命名規則を使用することで、コードの意図が明確になります。

Suggested change

	for (char e : s.toCharArray()) {
	if (hashMap.containsKey(e)) {
	Integer tmp = hashMap.get(e);
	tmp++;
	hashMap.put(e, tmp);
	} else {
	hashMap.put(e, 1);
	}
	}
	for (Integer i = 0; i < s.length(); i++) {
	char e = s.charAt(i);
	if (hashMap.get(e) == 1) {
	for (char c : s.toCharArray()) {
	if (hashMap.containsKey(c)) {
	Integer tmp = hashMap.get(c);
	tmp++;
	hashMap.put(c, tmp);
	} else {
	hashMap.put(c, 1);
	}
	}
	for (Integer i = 0; i < s.length(); i++) {
	char c = s.charAt(i);
	if (hashMap.get(c) == 1) {

[Copilot]

変数名が不明瞭（-0.5点）

問題点： 変数名tmpは一時的な変数であることしか示しておらず、その役割が不明確です。より説明的な名前を使用すべきです。

現在のコード：

Integer tmp = hashMap.get(e);
tmp++;

改善案：

Integer count = hashMap.get(e);
count++;

ただし、先に指摘した通り、この部分全体をhashMap.put(e, hashMap.getOrDefault(e, 0) + 1)に置き換えることでこの変数自体が不要になります。

Suggested change

	Integer tmp = hashMap.get(e);
	tmp++;
	hashMap.put(e, tmp);
	Integer count = hashMap.get(e);
	count++;
	hashMap.put(e, count);

[Copilot]

エッジケース処理のバグ（-1点）

問題点： 文字列が1文字の場合、その文字は必ずユニークなので0を返すのは正しいですが、空文字列の場合も0を返してしまいます。LeetCodeの問題387では、ユニーク文字が見つからない場合は-1を返すべきです。空文字列にはユニーク文字が存在しないため、-1を返すべきです。

現在のコード：

if (s.length() <= 1) {
    return 0;
}

改善案：

if (s.isEmpty()) {
    return -1;
}
if (s.length() == 1) {
    return 0;
}

または、このエッジケース処理を完全に削除しても、後続のロジックで正しく処理されます。

Suggested change

	if (s.length() <= 1) {
	if (s.isEmpty()) {
	return -1;
	}
	if (s.length() == 1) {

[Copilot]

非効率なHashMap更新処理（-1点）

問題点： hashMap.get(e)を呼び出して値を取得し、変数に代入してインクリメントしていますが、この方法は冗長です。また、同じキーに対して2回のHashMap操作（getとput）を行っています。

現在のコード：

if (hashMap.containsKey(e)) {
    Integer tmp = hashMap.get(e);
    tmp++;
    hashMap.put(e, tmp);
} else {
    hashMap.put(e, 1);
}

改善案：

hashMap.put(e, hashMap.getOrDefault(e, 0) + 1);

このワンライナーで同じ処理をより簡潔かつ効率的に実装できます。

Suggested change

	if (hashMap.containsKey(e)) {
	Integer tmp = hashMap.get(e);
	tmp++;
	hashMap.put(e, tmp);
	} else {
	hashMap.put(e, 1);
	}
	hashMap.put(e, hashMap.getOrDefault(e, 0) + 1);

[Copilot]

ボクシング（Boxing）の不要な使用（-0.5点）

問題点： ループカウンターにInteger（オブジェクト型）を使用していますが、プリミティブ型のintを使用すべきです。Integerを使用すると、不要なボクシング/アンボクシング操作が発生し、パフォーマンスに影響します。

現在のコード：

for (Integer i = 0; i < s.length(); i++) {

改善案：

for (int i = 0; i < s.length(); i++) {

プリミティブ型を使用することで、メモリ効率とパフォーマンスが向上します。

Suggested change

	for (Integer i = 0; i < s.length(); i++) {
	for (int i = 0; i < s.length(); i++) {

[Copilot]

変数名の改善提案（-0.5点）

問題点： 変数名hashMapは冗長です。型名をそのまま変数名にするのは推奨されません。変数の役割を示すより意味のある名前を使用すべきです。

現在のコード：

Map<Character, Integer> hashMap = new HashMap<>();

改善案：

Map<Character, Integer> charCount = new HashMap<>();

または

Map<Character, Integer> frequencyMap = new HashMap<>();

変数の目的（文字の出現回数を記録する）を明確に示す名前にすることで、コードの可読性が向上します。

Suggested change

	Map<Character, Integer> hashMap = new HashMap<>();
	for (char e : s.toCharArray()) {
	if (hashMap.containsKey(e)) {
	Integer tmp = hashMap.get(e);
	tmp++;
	hashMap.put(e, tmp);
	} else {
	hashMap.put(e, 1);
	}
	}
	for (Integer i = 0; i < s.length(); i++) {
	char e = s.charAt(i);
	if (hashMap.get(e) == 1) {
	Map<Character, Integer> charCount = new HashMap<>();
	for (char e : s.toCharArray()) {
	if (charCount.containsKey(e)) {
	Integer tmp = charCount.get(e);
	tmp++;
	charCount.put(e, tmp);
	} else {
	charCount.put(e, 1);
	}
	}
	for (Integer i = 0; i < s.length(); i++) {
	char e = s.charAt(i);
	if (charCount.get(e) == 1) {