Поясните в чем ошибка на java я только учусь

задание на codewars
Create a function taking a positive integer as its parameter and returning a string containing the Roman Numeral representation of that integer.

Modern Roman numerals are written by expressing each digit separately starting with the left most digit and skipping any digit with a value of zero. In Roman numerals 1990 is rendered: 1000=M, 900=CM, 90=XC; resulting in MCMXC. 2008 is written as 2000=MM, 8=VIII; or MMVIII. 1666 uses each Roman symbol in descending order: MDCLX
вот код

public static String solution(int n) {
        map.put(1, "I");
        map.put(5, "V");
        map.put(10, "X");
        map.put(50, "L");
        map.put(100, "C");
        map.put(500, "D");
        map.put(1000, "M");

        numGen(n, map);
        while (!numbers.isEmpty()) {
            romeNumCreator(numbers.poll());
        }



        return sb.toString();
    }


    static Queue<Integer> numGen(int n, Map<Integer, String> map) {

        //int[] discharges = {1, 10, 100, 1000};
        String s = String.valueOf(n);
        int length = s.length();

        do {
            if (map.containsKey(n)) {
                numbers.add(n);
                break;
            }
            int temp = n / DISCHARGES[length - 1];
            if (temp == 0) {
                length--;
                continue;
            }
            n %= DISCHARGES[length - 1];

            int num = temp * DISCHARGES[length - 1];
            numbers.add(num);

            length--;


        } while (length > 0);

        return numbers;
    }

    static void romeNumCreator(int num) {

        int n = num;
        if (map.containsKey(n)) {
            sb.append(map.get(n));
            return;
        }
        int length = String.valueOf(n).length();
        int count = n / DISCHARGES[length - 1];
        if (count <= MAX_REPEAT) {
            while (count > 0) {
                sb.append(map.get(DISCHARGES[length - 1]));
                count--;
            }
        } else if (map.containsKey(n + DISCHARGES[length - 1])) {
            sb.append(map.get(DISCHARGES[length - 1])).append(map.get(n + DISCHARGES[length - 1]));



        } else {
            int sum = 0;
            int res = n;
            do {
                res -= DISCHARGES[length - 1];
                sum += DISCHARGES[length - 1];

            } while (!map.containsKey(res));
            sb.append(map.get(res));
            romeNumCreator(sum);
        }
    }
}

Вот что мне выдает test с codewars
ConversionTest

shouldConvertToRoman

solution(4) should equal to IV expected:<I[]V> but was:<I[I]V>
Вчем дело не могу понять вывод то правильный

Тут видимо написано, что вывело IIV вместо IV.


ЗЫ при вставке кода на форум надо было на пустую строку перейти перед нажатием кнопки Код )

но у меня вывод верный же

В смысле верный? Как проверяли?

Оно же пишет, что должно получиться IV (при вводе 4), а вывело IIV.

в моем коде выводит IV

откуда берется II я не пойму

как проверяли?

давал на вход 4 и на выходе получал iv

Скиньте весь код, в первом сообщении начало пропущено.

А это где на codewars? Тут я что-то не вижу Джавы https://www.codewars.com/kata/51b6249c4612257ac0000005/

public class Main {
    static final int MAX_REPEAT = 3;
    static final int[] DISCHARGES = {1, 10, 100, 1000};
    static Queue<Integer> numbers = new ArrayDeque<>();
    static StringBuilder sb = new StringBuilder();
    static Map<Integer, String> map = new HashMap<>();

    public static void main(String[] args) {
        //Map<Integer, Character> map = new HashMap<>();
        /*map.put(1, 'I');
        map.put(5, 'V');
        map.put(10, 'X');
        map.put(50, 'L');
        map.put(100, 'C');
        map.put(500, 'D');
        map.put(1000, 'M');

        numGen(3645, map);
        while (!numbers.isEmpty()) {
            romeNumCreator(numbers.poll());
        }
        System.out.println(sb.toString());

*/

        System.out.println(solution(6));
    }
    public static String solution(int n) {
        map.put(1, "I");
        map.put(5, "V");
        map.put(10, "X");
        map.put(50, "L");
        map.put(100, "C");
        map.put(500, "D");
        map.put(1000, "M");

        numGen(n, map);
        while (!numbers.isEmpty()) {
            romeNumCreator(numbers.poll());
        }



        return sb.toString();
    }


    static Queue<Integer> numGen(int n, Map<Integer, String> map) {

        //int[] discharges = {1, 10, 100, 1000};
        String s = String.valueOf(n);
        int length = s.length();

        do {
            if (map.containsKey(n)) {
                numbers.add(n);
                break;
            }
            int temp = n / DISCHARGES[length - 1];
            if (temp == 0) {
                length--;
                continue;
            }
            n %= DISCHARGES[length - 1];

            int num = temp * DISCHARGES[length - 1];
            numbers.add(num);

            length--;


        } while (length > 0);

        return numbers;
    }

    static void romeNumCreator(int num) {

        int n = num;
        if (map.containsKey(n)) {
            sb.append(map.get(n));
            return;
        }
        int length = String.valueOf(n).length();
        int count = n / DISCHARGES[length - 1];
        if (count <= MAX_REPEAT) {
            while (count > 0) {
                sb.append(map.get(DISCHARGES[length - 1]));
                count--;
            }
        } else if (map.containsKey(n + DISCHARGES[length - 1])) {
            sb.append(map.get(DISCHARGES[length - 1])).append(map.get(n + DISCHARGES[length - 1]));



        } else {
            int sum = 0;
            int res = n;
            do {
                res -= DISCHARGES[length - 1];
                sum += DISCHARGES[length - 1];

            } while (!map.containsKey(res));
            sb.append(map.get(res));
            romeNumCreator(sum);
        }




    }


}

А так? :slight_smile:

        System.out.println(solution(1));
        System.out.println(solution(4));

Это пример того, почему по возможности нужно избегать любого глобального состояния.

Чистые функции рулят.

https://ru.wikipedia.org/wiki/Ссылочная_прозрачность
https://en.wikipedia.org/wiki/Immutable_object

Спасибо вам!!!
Я исправил, блин ну и сложная задачка…
вот что получилось

import java.util.ArrayDeque;
import java.util.HashMap;
import java.util.Map;
import java.util.Queue;

public class Main {
    static final int MAX_REPEAT = 3;
    static final int[] DISCHARGES = {1, 10, 100, 1000};
    static Queue<Integer> numbers = new ArrayDeque<>();
    static StringBuilder sb;
    static Map<Integer, String> map = new HashMap<>();

    public static void main(String[] args) {
        //Map<Integer, Character> map = new HashMap<>();
        /*map.put(1, 'I');
        map.put(5, 'V');
        map.put(10, 'X');
        map.put(50, 'L');
        map.put(100, 'C');
        map.put(500, 'D');
        map.put(1000, 'M');

        numGen(3645, map);
        while (!numbers.isEmpty()) {
            romeNumCreator(numbers.poll());
        }
        System.out.println(sb.toString());

*/

        System.out.println(solution(1));
        System.out.println(solution(4));
    }

    public static String solution(int n) {
        sb = new StringBuilder();
        map.put(1, "I");
        map.put(5, "V");
        map.put(10, "X");
        map.put(50, "L");
        map.put(100, "C");
        map.put(500, "D");
        map.put(1000, "M");

        numGen(n, map);
        while (!numbers.isEmpty()) {
            romeNumCreator(numbers.poll());
        }


        return sb.toString();
    }


    static Queue<Integer> numGen(int n, Map<Integer, String> map) {

        //int[] discharges = {1, 10, 100, 1000};
        String s = String.valueOf(n);
        int length = s.length();

        do {
            if (map.containsKey(n)) {
                numbers.add(n);
                break;
            }
            int temp = n / DISCHARGES[length - 1];
            if (temp == 0) {
                length--;
                continue;
            }
            n %= DISCHARGES[length - 1];

            int num = temp * DISCHARGES[length - 1];
            numbers.add(num);

            length--;


        } while (length > 0);

        return numbers;
    }

    static void romeNumCreator(int num) {

        int n = num;
        if (map.containsKey(n)) {
            sb.append(map.get(n));
            return;
        }
        int length = String.valueOf(n).length();
        int count = n / DISCHARGES[length - 1];
        if (count <= MAX_REPEAT) {
            while (count > 0) {
                sb.append(map.get(DISCHARGES[length - 1]));
                count--;
            }
        } else if (map.containsKey(n + DISCHARGES[length - 1])) {
            sb.append(map.get(DISCHARGES[length - 1])).append(map.get(n + DISCHARGES[length - 1]));


        } else {
            int sum = 0;
            int res = n;
            do {
                res -= DISCHARGES[length - 1];
                sum += DISCHARGES[length - 1];

            } while (!map.containsKey(res));
            sb.append(map.get(res));
            romeNumCreator(sum);
        }


    }


}

критикуйте, помоему так как я исправил не пишется, это не красиво))

Можно просто это всё сделать переменными внутри функций

И заодно использовать тут возвращаемое значение

Queue<Integer> numbers = numGen...

(то есть внутри numGen создавать эту Queue)


И еще неплохо бы улучшить имена. :arrow_down:

https://ru.hexlet.io/blog/posts/naming-in-programming

У меня просто рекурсия внутри romeNumCreator

sb можно просто передавать параметром, это объект, они передаются по ссылке.

map можно оставить глобальным.
В идеале его тут можно сделать неизменяемым (например после создания в конструкторе класса, сделав всё не static).