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

Java
#1

задание на 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>
Вчем дело не могу понять вывод то правильный

Снимок экрана от 2020-09-06 16-50-08
Снимок экрана от 2020-09-06 16-50-081280×1024 138 KB

#2

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

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

#3

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

#4

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

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

#5

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

#6

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

#7

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

#8

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

#9

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

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

#10 
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);
        }




    }


}
#11
#12

А так? :slight_smile:

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

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

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

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

#13

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

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);
        }


    }


}

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

#14

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

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

Queue<Integer> numbers = numGen...

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

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

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

#15

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

#16

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

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