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

[res3ukt (Alex)]

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

[AlexP]

@res3ukt

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

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

---

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

[res3ukt (Alex)]

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

[AlexP]

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

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

[res3ukt (Alex)]

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

[res3ukt (Alex)]

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

[AlexP]

@res3ukt

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

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

[res3ukt (Alex)]

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

[AlexP]

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

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

[res3ukt (Alex)]

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




    }


}

[res3ukt (Alex)]

Roman Numerals Encoder

[AlexP]

А так?

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

---

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

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

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

[res3ukt (Alex)]

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

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


    }


}

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

[AlexP]

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

@res3ukt

    static Queue<Integer> numbers = new ArrayDeque<>();
    static StringBuilder sb;
    static Map<Integer, String> map = new HashMap<>();

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

@res3ukt

numGen(n, map);

Queue<Integer> numbers = numGen...

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

---

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

@AlexP

• Несколько статей и лекций об именовании (переменных, функций, …) в программировании, ошибках именования

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

[res3ukt (Alex)]

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

[AlexP]

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

@res3ukt

    static void romeNumCreator(int num, StringBuilder result) {
            ...
            romeNumCreator(sum, result);
    }

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