Integer to Roman Numerals (1 - 2999)












1














This is a homework assignment. Convert an integer (1 - 2999) to roman numerals. I was wondering if I could make my code more efficient. The way I have it set up is that I take the digits of the input (the thousands, hundreds, tens, ones) and have cases for each of them. However, the approach I took to this problem is that there are a lot of if statements. My professor prefers that I utilize nothing that is not taught yet. I can only use for, if, while statements - the basics of Java. I was wondering if there are different approaches I could take to this problem so that the code is much more shorter as it is relatively long.



class romanNum {
public static void main (String args) {
//Initialize all variables including Scanner
Scanner input = new Scanner (System.in) ;
int userInput;
int onesDigit ;
int tensDigit ;
int hundredsDigit ;
int thousandsDigit ;
String one = "I";
String five = "V";
String ten = "X";
String fifty = "L";
String hundred = "C";
String fiveHundred = "D";
String thousand = "M";
boolean keepGoing = true ;

//User Input
userInput = input.nextInt();

//Seperate the digits
thousandsDigit = (userInput/1000) % 10;
hundredsDigit = (userInput / 100) % 10;
tensDigit = (userInput / 10) % 10;
onesDigit = (userInput / 1) % 10;
//Thousands (Because range is 1 - 2999, i didnt do more cases for thousands
while (keepGoing) {
for (int count = 0 ; count < thousandsDigit ; count++) {
System.out.print (thousand);
}

//Hundreds (Checks all cases 1-9)
//Checks 1 - 3
if ((hundredsDigit >= 1) && (hundredsDigit <= 3)) {
for (int count = 0 ; count < hundredsDigit ; count++) {
System.out.print (hundred);
}
}
//Checks 4
else if (hundredsDigit == 4) {
System.out.print (hundred + fiveHundred);
}
// Checks 9
else if (hundredsDigit == 9) {
System.out.print (hundred + thousand);
}
//Omit 0
else if (hundredsDigit == 0) {
}
// Checks 5 - 8
else {
System.out.print (fiveHundred);
for (int count = 0 ; count < hundredsDigit - 5 ; count++ ) {
System.out.print (hundred);
}
}

//Tens (Checks all cases 1-9)
//Checks 1 - 3
if ((tensDigit >= 1) && (tensDigit <= 3)) {
for (int count = 0 ; count < tensDigit ; count++) {
System.out.print (ten);
}
}
// Checks 4
else if (tensDigit == 4) {
System.out.print (ten + fifty);
}
// Checks 9
else if (tensDigit == 9) {
System.out.print (ten + hundred);
}
// Omit 0
else if (tensDigit == 0) {
}
// Checks 5 - 8
else {
System.out.print (fifty);
for (int count = 0 ; count < tensDigit - 5 ; count++ ) {
System.out.print (ten);
}
}

//Ones (Checks all cases 1-9)
//Checks 1 - 3
if ((onesDigit >= 1) && (onesDigit <= 3)) {
for (int count = 0 ; count < onesDigit ; count++) {
System.out.print (one);
}
}
// Checks 4
else if (onesDigit == 4) {
System.out.print (one + five);
}
//Checks 9
else if (onesDigit == 9) {
System.out.print (one + ten);
}
//Omit 0
else if (onesDigit == 0) {
}
// else (checks 5 - 9)
else {
System.out.print (five);
for (int count = 0 ; count < onesDigit - 5 ; count++ ) {
System.out.print (one);
}
}
//Print line outside of all conditions
System.out.println("");
keepGoing = false ;
}
}
}









share|improve this question





























    1














    This is a homework assignment. Convert an integer (1 - 2999) to roman numerals. I was wondering if I could make my code more efficient. The way I have it set up is that I take the digits of the input (the thousands, hundreds, tens, ones) and have cases for each of them. However, the approach I took to this problem is that there are a lot of if statements. My professor prefers that I utilize nothing that is not taught yet. I can only use for, if, while statements - the basics of Java. I was wondering if there are different approaches I could take to this problem so that the code is much more shorter as it is relatively long.



    class romanNum {
    public static void main (String args) {
    //Initialize all variables including Scanner
    Scanner input = new Scanner (System.in) ;
    int userInput;
    int onesDigit ;
    int tensDigit ;
    int hundredsDigit ;
    int thousandsDigit ;
    String one = "I";
    String five = "V";
    String ten = "X";
    String fifty = "L";
    String hundred = "C";
    String fiveHundred = "D";
    String thousand = "M";
    boolean keepGoing = true ;

    //User Input
    userInput = input.nextInt();

    //Seperate the digits
    thousandsDigit = (userInput/1000) % 10;
    hundredsDigit = (userInput / 100) % 10;
    tensDigit = (userInput / 10) % 10;
    onesDigit = (userInput / 1) % 10;
    //Thousands (Because range is 1 - 2999, i didnt do more cases for thousands
    while (keepGoing) {
    for (int count = 0 ; count < thousandsDigit ; count++) {
    System.out.print (thousand);
    }

    //Hundreds (Checks all cases 1-9)
    //Checks 1 - 3
    if ((hundredsDigit >= 1) && (hundredsDigit <= 3)) {
    for (int count = 0 ; count < hundredsDigit ; count++) {
    System.out.print (hundred);
    }
    }
    //Checks 4
    else if (hundredsDigit == 4) {
    System.out.print (hundred + fiveHundred);
    }
    // Checks 9
    else if (hundredsDigit == 9) {
    System.out.print (hundred + thousand);
    }
    //Omit 0
    else if (hundredsDigit == 0) {
    }
    // Checks 5 - 8
    else {
    System.out.print (fiveHundred);
    for (int count = 0 ; count < hundredsDigit - 5 ; count++ ) {
    System.out.print (hundred);
    }
    }

    //Tens (Checks all cases 1-9)
    //Checks 1 - 3
    if ((tensDigit >= 1) && (tensDigit <= 3)) {
    for (int count = 0 ; count < tensDigit ; count++) {
    System.out.print (ten);
    }
    }
    // Checks 4
    else if (tensDigit == 4) {
    System.out.print (ten + fifty);
    }
    // Checks 9
    else if (tensDigit == 9) {
    System.out.print (ten + hundred);
    }
    // Omit 0
    else if (tensDigit == 0) {
    }
    // Checks 5 - 8
    else {
    System.out.print (fifty);
    for (int count = 0 ; count < tensDigit - 5 ; count++ ) {
    System.out.print (ten);
    }
    }

    //Ones (Checks all cases 1-9)
    //Checks 1 - 3
    if ((onesDigit >= 1) && (onesDigit <= 3)) {
    for (int count = 0 ; count < onesDigit ; count++) {
    System.out.print (one);
    }
    }
    // Checks 4
    else if (onesDigit == 4) {
    System.out.print (one + five);
    }
    //Checks 9
    else if (onesDigit == 9) {
    System.out.print (one + ten);
    }
    //Omit 0
    else if (onesDigit == 0) {
    }
    // else (checks 5 - 9)
    else {
    System.out.print (five);
    for (int count = 0 ; count < onesDigit - 5 ; count++ ) {
    System.out.print (one);
    }
    }
    //Print line outside of all conditions
    System.out.println("");
    keepGoing = false ;
    }
    }
    }









    share|improve this question



























      1












      1








      1







      This is a homework assignment. Convert an integer (1 - 2999) to roman numerals. I was wondering if I could make my code more efficient. The way I have it set up is that I take the digits of the input (the thousands, hundreds, tens, ones) and have cases for each of them. However, the approach I took to this problem is that there are a lot of if statements. My professor prefers that I utilize nothing that is not taught yet. I can only use for, if, while statements - the basics of Java. I was wondering if there are different approaches I could take to this problem so that the code is much more shorter as it is relatively long.



      class romanNum {
      public static void main (String args) {
      //Initialize all variables including Scanner
      Scanner input = new Scanner (System.in) ;
      int userInput;
      int onesDigit ;
      int tensDigit ;
      int hundredsDigit ;
      int thousandsDigit ;
      String one = "I";
      String five = "V";
      String ten = "X";
      String fifty = "L";
      String hundred = "C";
      String fiveHundred = "D";
      String thousand = "M";
      boolean keepGoing = true ;

      //User Input
      userInput = input.nextInt();

      //Seperate the digits
      thousandsDigit = (userInput/1000) % 10;
      hundredsDigit = (userInput / 100) % 10;
      tensDigit = (userInput / 10) % 10;
      onesDigit = (userInput / 1) % 10;
      //Thousands (Because range is 1 - 2999, i didnt do more cases for thousands
      while (keepGoing) {
      for (int count = 0 ; count < thousandsDigit ; count++) {
      System.out.print (thousand);
      }

      //Hundreds (Checks all cases 1-9)
      //Checks 1 - 3
      if ((hundredsDigit >= 1) && (hundredsDigit <= 3)) {
      for (int count = 0 ; count < hundredsDigit ; count++) {
      System.out.print (hundred);
      }
      }
      //Checks 4
      else if (hundredsDigit == 4) {
      System.out.print (hundred + fiveHundred);
      }
      // Checks 9
      else if (hundredsDigit == 9) {
      System.out.print (hundred + thousand);
      }
      //Omit 0
      else if (hundredsDigit == 0) {
      }
      // Checks 5 - 8
      else {
      System.out.print (fiveHundred);
      for (int count = 0 ; count < hundredsDigit - 5 ; count++ ) {
      System.out.print (hundred);
      }
      }

      //Tens (Checks all cases 1-9)
      //Checks 1 - 3
      if ((tensDigit >= 1) && (tensDigit <= 3)) {
      for (int count = 0 ; count < tensDigit ; count++) {
      System.out.print (ten);
      }
      }
      // Checks 4
      else if (tensDigit == 4) {
      System.out.print (ten + fifty);
      }
      // Checks 9
      else if (tensDigit == 9) {
      System.out.print (ten + hundred);
      }
      // Omit 0
      else if (tensDigit == 0) {
      }
      // Checks 5 - 8
      else {
      System.out.print (fifty);
      for (int count = 0 ; count < tensDigit - 5 ; count++ ) {
      System.out.print (ten);
      }
      }

      //Ones (Checks all cases 1-9)
      //Checks 1 - 3
      if ((onesDigit >= 1) && (onesDigit <= 3)) {
      for (int count = 0 ; count < onesDigit ; count++) {
      System.out.print (one);
      }
      }
      // Checks 4
      else if (onesDigit == 4) {
      System.out.print (one + five);
      }
      //Checks 9
      else if (onesDigit == 9) {
      System.out.print (one + ten);
      }
      //Omit 0
      else if (onesDigit == 0) {
      }
      // else (checks 5 - 9)
      else {
      System.out.print (five);
      for (int count = 0 ; count < onesDigit - 5 ; count++ ) {
      System.out.print (one);
      }
      }
      //Print line outside of all conditions
      System.out.println("");
      keepGoing = false ;
      }
      }
      }









      share|improve this question















      This is a homework assignment. Convert an integer (1 - 2999) to roman numerals. I was wondering if I could make my code more efficient. The way I have it set up is that I take the digits of the input (the thousands, hundreds, tens, ones) and have cases for each of them. However, the approach I took to this problem is that there are a lot of if statements. My professor prefers that I utilize nothing that is not taught yet. I can only use for, if, while statements - the basics of Java. I was wondering if there are different approaches I could take to this problem so that the code is much more shorter as it is relatively long.



      class romanNum {
      public static void main (String args) {
      //Initialize all variables including Scanner
      Scanner input = new Scanner (System.in) ;
      int userInput;
      int onesDigit ;
      int tensDigit ;
      int hundredsDigit ;
      int thousandsDigit ;
      String one = "I";
      String five = "V";
      String ten = "X";
      String fifty = "L";
      String hundred = "C";
      String fiveHundred = "D";
      String thousand = "M";
      boolean keepGoing = true ;

      //User Input
      userInput = input.nextInt();

      //Seperate the digits
      thousandsDigit = (userInput/1000) % 10;
      hundredsDigit = (userInput / 100) % 10;
      tensDigit = (userInput / 10) % 10;
      onesDigit = (userInput / 1) % 10;
      //Thousands (Because range is 1 - 2999, i didnt do more cases for thousands
      while (keepGoing) {
      for (int count = 0 ; count < thousandsDigit ; count++) {
      System.out.print (thousand);
      }

      //Hundreds (Checks all cases 1-9)
      //Checks 1 - 3
      if ((hundredsDigit >= 1) && (hundredsDigit <= 3)) {
      for (int count = 0 ; count < hundredsDigit ; count++) {
      System.out.print (hundred);
      }
      }
      //Checks 4
      else if (hundredsDigit == 4) {
      System.out.print (hundred + fiveHundred);
      }
      // Checks 9
      else if (hundredsDigit == 9) {
      System.out.print (hundred + thousand);
      }
      //Omit 0
      else if (hundredsDigit == 0) {
      }
      // Checks 5 - 8
      else {
      System.out.print (fiveHundred);
      for (int count = 0 ; count < hundredsDigit - 5 ; count++ ) {
      System.out.print (hundred);
      }
      }

      //Tens (Checks all cases 1-9)
      //Checks 1 - 3
      if ((tensDigit >= 1) && (tensDigit <= 3)) {
      for (int count = 0 ; count < tensDigit ; count++) {
      System.out.print (ten);
      }
      }
      // Checks 4
      else if (tensDigit == 4) {
      System.out.print (ten + fifty);
      }
      // Checks 9
      else if (tensDigit == 9) {
      System.out.print (ten + hundred);
      }
      // Omit 0
      else if (tensDigit == 0) {
      }
      // Checks 5 - 8
      else {
      System.out.print (fifty);
      for (int count = 0 ; count < tensDigit - 5 ; count++ ) {
      System.out.print (ten);
      }
      }

      //Ones (Checks all cases 1-9)
      //Checks 1 - 3
      if ((onesDigit >= 1) && (onesDigit <= 3)) {
      for (int count = 0 ; count < onesDigit ; count++) {
      System.out.print (one);
      }
      }
      // Checks 4
      else if (onesDigit == 4) {
      System.out.print (one + five);
      }
      //Checks 9
      else if (onesDigit == 9) {
      System.out.print (one + ten);
      }
      //Omit 0
      else if (onesDigit == 0) {
      }
      // else (checks 5 - 9)
      else {
      System.out.print (five);
      for (int count = 0 ; count < onesDigit - 5 ; count++ ) {
      System.out.print (one);
      }
      }
      //Print line outside of all conditions
      System.out.println("");
      keepGoing = false ;
      }
      }
      }






      java beginner homework roman-numerals






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited Oct 16 at 6:04









      200_success

      128k15149412




      128k15149412










      asked Oct 16 at 5:40









      Henry So

      61




      61






















          2 Answers
          2






          active

          oldest

          votes


















          0














          Because of the repeating scheme 1er, 5er, we can do it in a loop. The special cases 0/5 and 4/9 are as like as your code.



          private static String toRomanNum(int val) {
          String out = "", chars = "IVXLCDM";
          int i, digit, d5, m5, idx=6, divi = 1000;

          while(idx>=0) {
          digit = val/divi;
          val %= divi;
          d5=digit/5;
          m5=digit%5;
          if(m5==0) {
          if(d5==1) out += chars.charAt(idx+1);
          }
          else if(m5==4) out += chars.charAt(idx) +""+ chars.charAt(idx+1+d5);
          else {
          if(d5==1) out += chars.charAt(idx+1);
          for(i=0;i<m5;i++) out += chars.charAt(idx);
          }
          divi/=10;
          idx-=2;
          }
          return out;
          }





          share|improve this answer





























            0














            Control structure formatting




                        }
            // Checks 4
            else if (onesDigit == 4) {
            System.out.print (one + five);



            Please don't put comments in between curly braces and their associated control statements.



                        }
            else if (onesDigit == 4) {
            // Checks 4
            System.out.print (one + five);


            Just treat them as one unit. The problem with separating them is that it looks like the structure is over, because Java (and other C-style languages) use the same symbol for ending the control structure as ending the current piece of the structure.



            I would actually write it as



                       } else if (onesDigit == 4) {


            Which is also the Java standard. But that's not as important as not adding more stuff between.



            Comments



            Most of your comments just restate what the code does. That makes sense in class, as the professor is telling you what the code does to help you learn. And since you may not understand what the code does, those kind of comments can make your programs easier to grade. But outside class, we expect programmers to be able to read code. Comments should not tell us what the code does, but why it is doing it.



            Declarations



            It is also the standard in Java to declare variables as late as possible, often at initialization. Your method looks more like old C code with all the declarations at the beginning.




            try-with-resources



            This may be past what your assignment allowed, but the right way to use an Autocloseable resource like a Scanner is



            try (Scanner input = new Scanner(System.in)) {
            // do stuff with the Scanner
            }


            This will automatically close the Scanner when it is done, even if there is an exception. It's essentially the equivalent of adding



            } finally {
            if (input != null) {
            input.close();
            }


            I would also avoid putting spaces between a method name and the opening parenthesis. E.g. Scanner(System.in) is a constructor call. I would put a space in if (.



            Putting it together



            Adding the method from this answer (albeit with code that I find more readable):



            class RomanNumeral {

            private static final String DIGITS = "IVXLCDM";

            public static void main(String args) {
            try (Scanner input = new Scanner(System.in)) {
            System.out.println(toRomanNumeral(input.nextInt()));
            }
            }

            private static String toRomanNumeral(int value) {
            StringBuilder out = new StringBuilder();

            int divisor = 1000;
            for (int i = DIGITS.length() - 1; i >= 0; i -= 2) {
            int unit = value / divisor;
            value %= divisor;

            int unit5 = unit / 5;
            int remainder = unit % 5;

            if (remainder == 4) {
            // if the remainder is 4, then one of IV, IX, XL, XC, CD, CM
            // i tells us if it is I, X, or C
            // i + 1 tells us which pair: VX, LC, or DM.
            // unit5 tells us if it is the first (V, L, or D) or second (X, C, or M)
            out.append(DIGITS.charAt(i))
            .append(DIGITS.charAt(i + 1 + unit5));
            } else {
            // add V, L, or D if needed
            if (unit5 == 1) {
            out.append(DIGITS.charAt(i + 1));
            }

            // add up to three I, X, or C
            for (int j = 0; j < remainder; j++) {
            out.append(DIGITS.charAt(i));
            }
            }

            divisor /= 10;
            }

            return out.toString();
            }

            }


            Your professor also might bar StringBuilder, although he shouldn't. Mixing calculation and output the way that your program does is a code smell. StringBuilder should be taught before System.out.print in my opinion. That said, the algorithm doesn't change if you replace the append calls with System.out.print.



            The initial check if there is no remainder was unnecessary. The else case will handle it correctly. So I removed it.



            I used a for loop as shorter and more readable than a while loop.



            Moving the code to a separate method is simply better practice. Again, if your professor has not taught you that, that is a teaching failure. IMnsHO, method calls should be taught before if, else, and loops (for or while).



            This code will throw an exception if given an input of 3900 or more (also probably for negative values). The exception will be confusing. It would be better to check for invalid values and throw a more sensible exception. However, your professor may not have taught exceptions yet.



            I made DIGITS a class constant because it never changes throughout the program. You can make it a method variable if your professor hasn't taught class constants yet.






            share|improve this answer





















              Your Answer





              StackExchange.ifUsing("editor", function () {
              return StackExchange.using("mathjaxEditing", function () {
              StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
              StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
              });
              });
              }, "mathjax-editing");

              StackExchange.ifUsing("editor", function () {
              StackExchange.using("externalEditor", function () {
              StackExchange.using("snippets", function () {
              StackExchange.snippets.init();
              });
              });
              }, "code-snippets");

              StackExchange.ready(function() {
              var channelOptions = {
              tags: "".split(" "),
              id: "196"
              };
              initTagRenderer("".split(" "), "".split(" "), channelOptions);

              StackExchange.using("externalEditor", function() {
              // Have to fire editor after snippets, if snippets enabled
              if (StackExchange.settings.snippets.snippetsEnabled) {
              StackExchange.using("snippets", function() {
              createEditor();
              });
              }
              else {
              createEditor();
              }
              });

              function createEditor() {
              StackExchange.prepareEditor({
              heartbeatType: 'answer',
              autoActivateHeartbeat: false,
              convertImagesToLinks: false,
              noModals: true,
              showLowRepImageUploadWarning: true,
              reputationToPostImages: null,
              bindNavPrevention: true,
              postfix: "",
              imageUploader: {
              brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
              contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
              allowUrls: true
              },
              onDemand: true,
              discardSelector: ".discard-answer"
              ,immediatelyShowMarkdownHelp:true
              });


              }
              });














              draft saved

              draft discarded


















              StackExchange.ready(
              function () {
              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f205650%2finteger-to-roman-numerals-1-2999%23new-answer', 'question_page');
              }
              );

              Post as a guest















              Required, but never shown

























              2 Answers
              2






              active

              oldest

              votes








              2 Answers
              2






              active

              oldest

              votes









              active

              oldest

              votes






              active

              oldest

              votes









              0














              Because of the repeating scheme 1er, 5er, we can do it in a loop. The special cases 0/5 and 4/9 are as like as your code.



              private static String toRomanNum(int val) {
              String out = "", chars = "IVXLCDM";
              int i, digit, d5, m5, idx=6, divi = 1000;

              while(idx>=0) {
              digit = val/divi;
              val %= divi;
              d5=digit/5;
              m5=digit%5;
              if(m5==0) {
              if(d5==1) out += chars.charAt(idx+1);
              }
              else if(m5==4) out += chars.charAt(idx) +""+ chars.charAt(idx+1+d5);
              else {
              if(d5==1) out += chars.charAt(idx+1);
              for(i=0;i<m5;i++) out += chars.charAt(idx);
              }
              divi/=10;
              idx-=2;
              }
              return out;
              }





              share|improve this answer


























                0














                Because of the repeating scheme 1er, 5er, we can do it in a loop. The special cases 0/5 and 4/9 are as like as your code.



                private static String toRomanNum(int val) {
                String out = "", chars = "IVXLCDM";
                int i, digit, d5, m5, idx=6, divi = 1000;

                while(idx>=0) {
                digit = val/divi;
                val %= divi;
                d5=digit/5;
                m5=digit%5;
                if(m5==0) {
                if(d5==1) out += chars.charAt(idx+1);
                }
                else if(m5==4) out += chars.charAt(idx) +""+ chars.charAt(idx+1+d5);
                else {
                if(d5==1) out += chars.charAt(idx+1);
                for(i=0;i<m5;i++) out += chars.charAt(idx);
                }
                divi/=10;
                idx-=2;
                }
                return out;
                }





                share|improve this answer
























                  0












                  0








                  0






                  Because of the repeating scheme 1er, 5er, we can do it in a loop. The special cases 0/5 and 4/9 are as like as your code.



                  private static String toRomanNum(int val) {
                  String out = "", chars = "IVXLCDM";
                  int i, digit, d5, m5, idx=6, divi = 1000;

                  while(idx>=0) {
                  digit = val/divi;
                  val %= divi;
                  d5=digit/5;
                  m5=digit%5;
                  if(m5==0) {
                  if(d5==1) out += chars.charAt(idx+1);
                  }
                  else if(m5==4) out += chars.charAt(idx) +""+ chars.charAt(idx+1+d5);
                  else {
                  if(d5==1) out += chars.charAt(idx+1);
                  for(i=0;i<m5;i++) out += chars.charAt(idx);
                  }
                  divi/=10;
                  idx-=2;
                  }
                  return out;
                  }





                  share|improve this answer












                  Because of the repeating scheme 1er, 5er, we can do it in a loop. The special cases 0/5 and 4/9 are as like as your code.



                  private static String toRomanNum(int val) {
                  String out = "", chars = "IVXLCDM";
                  int i, digit, d5, m5, idx=6, divi = 1000;

                  while(idx>=0) {
                  digit = val/divi;
                  val %= divi;
                  d5=digit/5;
                  m5=digit%5;
                  if(m5==0) {
                  if(d5==1) out += chars.charAt(idx+1);
                  }
                  else if(m5==4) out += chars.charAt(idx) +""+ chars.charAt(idx+1+d5);
                  else {
                  if(d5==1) out += chars.charAt(idx+1);
                  for(i=0;i<m5;i++) out += chars.charAt(idx);
                  }
                  divi/=10;
                  idx-=2;
                  }
                  return out;
                  }






                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered Oct 16 at 11:54









                  Holger

                  20613




                  20613

























                      0














                      Control structure formatting




                                  }
                      // Checks 4
                      else if (onesDigit == 4) {
                      System.out.print (one + five);



                      Please don't put comments in between curly braces and their associated control statements.



                                  }
                      else if (onesDigit == 4) {
                      // Checks 4
                      System.out.print (one + five);


                      Just treat them as one unit. The problem with separating them is that it looks like the structure is over, because Java (and other C-style languages) use the same symbol for ending the control structure as ending the current piece of the structure.



                      I would actually write it as



                                 } else if (onesDigit == 4) {


                      Which is also the Java standard. But that's not as important as not adding more stuff between.



                      Comments



                      Most of your comments just restate what the code does. That makes sense in class, as the professor is telling you what the code does to help you learn. And since you may not understand what the code does, those kind of comments can make your programs easier to grade. But outside class, we expect programmers to be able to read code. Comments should not tell us what the code does, but why it is doing it.



                      Declarations



                      It is also the standard in Java to declare variables as late as possible, often at initialization. Your method looks more like old C code with all the declarations at the beginning.




                      try-with-resources



                      This may be past what your assignment allowed, but the right way to use an Autocloseable resource like a Scanner is



                      try (Scanner input = new Scanner(System.in)) {
                      // do stuff with the Scanner
                      }


                      This will automatically close the Scanner when it is done, even if there is an exception. It's essentially the equivalent of adding



                      } finally {
                      if (input != null) {
                      input.close();
                      }


                      I would also avoid putting spaces between a method name and the opening parenthesis. E.g. Scanner(System.in) is a constructor call. I would put a space in if (.



                      Putting it together



                      Adding the method from this answer (albeit with code that I find more readable):



                      class RomanNumeral {

                      private static final String DIGITS = "IVXLCDM";

                      public static void main(String args) {
                      try (Scanner input = new Scanner(System.in)) {
                      System.out.println(toRomanNumeral(input.nextInt()));
                      }
                      }

                      private static String toRomanNumeral(int value) {
                      StringBuilder out = new StringBuilder();

                      int divisor = 1000;
                      for (int i = DIGITS.length() - 1; i >= 0; i -= 2) {
                      int unit = value / divisor;
                      value %= divisor;

                      int unit5 = unit / 5;
                      int remainder = unit % 5;

                      if (remainder == 4) {
                      // if the remainder is 4, then one of IV, IX, XL, XC, CD, CM
                      // i tells us if it is I, X, or C
                      // i + 1 tells us which pair: VX, LC, or DM.
                      // unit5 tells us if it is the first (V, L, or D) or second (X, C, or M)
                      out.append(DIGITS.charAt(i))
                      .append(DIGITS.charAt(i + 1 + unit5));
                      } else {
                      // add V, L, or D if needed
                      if (unit5 == 1) {
                      out.append(DIGITS.charAt(i + 1));
                      }

                      // add up to three I, X, or C
                      for (int j = 0; j < remainder; j++) {
                      out.append(DIGITS.charAt(i));
                      }
                      }

                      divisor /= 10;
                      }

                      return out.toString();
                      }

                      }


                      Your professor also might bar StringBuilder, although he shouldn't. Mixing calculation and output the way that your program does is a code smell. StringBuilder should be taught before System.out.print in my opinion. That said, the algorithm doesn't change if you replace the append calls with System.out.print.



                      The initial check if there is no remainder was unnecessary. The else case will handle it correctly. So I removed it.



                      I used a for loop as shorter and more readable than a while loop.



                      Moving the code to a separate method is simply better practice. Again, if your professor has not taught you that, that is a teaching failure. IMnsHO, method calls should be taught before if, else, and loops (for or while).



                      This code will throw an exception if given an input of 3900 or more (also probably for negative values). The exception will be confusing. It would be better to check for invalid values and throw a more sensible exception. However, your professor may not have taught exceptions yet.



                      I made DIGITS a class constant because it never changes throughout the program. You can make it a method variable if your professor hasn't taught class constants yet.






                      share|improve this answer


























                        0














                        Control structure formatting




                                    }
                        // Checks 4
                        else if (onesDigit == 4) {
                        System.out.print (one + five);



                        Please don't put comments in between curly braces and their associated control statements.



                                    }
                        else if (onesDigit == 4) {
                        // Checks 4
                        System.out.print (one + five);


                        Just treat them as one unit. The problem with separating them is that it looks like the structure is over, because Java (and other C-style languages) use the same symbol for ending the control structure as ending the current piece of the structure.



                        I would actually write it as



                                   } else if (onesDigit == 4) {


                        Which is also the Java standard. But that's not as important as not adding more stuff between.



                        Comments



                        Most of your comments just restate what the code does. That makes sense in class, as the professor is telling you what the code does to help you learn. And since you may not understand what the code does, those kind of comments can make your programs easier to grade. But outside class, we expect programmers to be able to read code. Comments should not tell us what the code does, but why it is doing it.



                        Declarations



                        It is also the standard in Java to declare variables as late as possible, often at initialization. Your method looks more like old C code with all the declarations at the beginning.




                        try-with-resources



                        This may be past what your assignment allowed, but the right way to use an Autocloseable resource like a Scanner is



                        try (Scanner input = new Scanner(System.in)) {
                        // do stuff with the Scanner
                        }


                        This will automatically close the Scanner when it is done, even if there is an exception. It's essentially the equivalent of adding



                        } finally {
                        if (input != null) {
                        input.close();
                        }


                        I would also avoid putting spaces between a method name and the opening parenthesis. E.g. Scanner(System.in) is a constructor call. I would put a space in if (.



                        Putting it together



                        Adding the method from this answer (albeit with code that I find more readable):



                        class RomanNumeral {

                        private static final String DIGITS = "IVXLCDM";

                        public static void main(String args) {
                        try (Scanner input = new Scanner(System.in)) {
                        System.out.println(toRomanNumeral(input.nextInt()));
                        }
                        }

                        private static String toRomanNumeral(int value) {
                        StringBuilder out = new StringBuilder();

                        int divisor = 1000;
                        for (int i = DIGITS.length() - 1; i >= 0; i -= 2) {
                        int unit = value / divisor;
                        value %= divisor;

                        int unit5 = unit / 5;
                        int remainder = unit % 5;

                        if (remainder == 4) {
                        // if the remainder is 4, then one of IV, IX, XL, XC, CD, CM
                        // i tells us if it is I, X, or C
                        // i + 1 tells us which pair: VX, LC, or DM.
                        // unit5 tells us if it is the first (V, L, or D) or second (X, C, or M)
                        out.append(DIGITS.charAt(i))
                        .append(DIGITS.charAt(i + 1 + unit5));
                        } else {
                        // add V, L, or D if needed
                        if (unit5 == 1) {
                        out.append(DIGITS.charAt(i + 1));
                        }

                        // add up to three I, X, or C
                        for (int j = 0; j < remainder; j++) {
                        out.append(DIGITS.charAt(i));
                        }
                        }

                        divisor /= 10;
                        }

                        return out.toString();
                        }

                        }


                        Your professor also might bar StringBuilder, although he shouldn't. Mixing calculation and output the way that your program does is a code smell. StringBuilder should be taught before System.out.print in my opinion. That said, the algorithm doesn't change if you replace the append calls with System.out.print.



                        The initial check if there is no remainder was unnecessary. The else case will handle it correctly. So I removed it.



                        I used a for loop as shorter and more readable than a while loop.



                        Moving the code to a separate method is simply better practice. Again, if your professor has not taught you that, that is a teaching failure. IMnsHO, method calls should be taught before if, else, and loops (for or while).



                        This code will throw an exception if given an input of 3900 or more (also probably for negative values). The exception will be confusing. It would be better to check for invalid values and throw a more sensible exception. However, your professor may not have taught exceptions yet.



                        I made DIGITS a class constant because it never changes throughout the program. You can make it a method variable if your professor hasn't taught class constants yet.






                        share|improve this answer
























                          0












                          0








                          0






                          Control structure formatting




                                      }
                          // Checks 4
                          else if (onesDigit == 4) {
                          System.out.print (one + five);



                          Please don't put comments in between curly braces and their associated control statements.



                                      }
                          else if (onesDigit == 4) {
                          // Checks 4
                          System.out.print (one + five);


                          Just treat them as one unit. The problem with separating them is that it looks like the structure is over, because Java (and other C-style languages) use the same symbol for ending the control structure as ending the current piece of the structure.



                          I would actually write it as



                                     } else if (onesDigit == 4) {


                          Which is also the Java standard. But that's not as important as not adding more stuff between.



                          Comments



                          Most of your comments just restate what the code does. That makes sense in class, as the professor is telling you what the code does to help you learn. And since you may not understand what the code does, those kind of comments can make your programs easier to grade. But outside class, we expect programmers to be able to read code. Comments should not tell us what the code does, but why it is doing it.



                          Declarations



                          It is also the standard in Java to declare variables as late as possible, often at initialization. Your method looks more like old C code with all the declarations at the beginning.




                          try-with-resources



                          This may be past what your assignment allowed, but the right way to use an Autocloseable resource like a Scanner is



                          try (Scanner input = new Scanner(System.in)) {
                          // do stuff with the Scanner
                          }


                          This will automatically close the Scanner when it is done, even if there is an exception. It's essentially the equivalent of adding



                          } finally {
                          if (input != null) {
                          input.close();
                          }


                          I would also avoid putting spaces between a method name and the opening parenthesis. E.g. Scanner(System.in) is a constructor call. I would put a space in if (.



                          Putting it together



                          Adding the method from this answer (albeit with code that I find more readable):



                          class RomanNumeral {

                          private static final String DIGITS = "IVXLCDM";

                          public static void main(String args) {
                          try (Scanner input = new Scanner(System.in)) {
                          System.out.println(toRomanNumeral(input.nextInt()));
                          }
                          }

                          private static String toRomanNumeral(int value) {
                          StringBuilder out = new StringBuilder();

                          int divisor = 1000;
                          for (int i = DIGITS.length() - 1; i >= 0; i -= 2) {
                          int unit = value / divisor;
                          value %= divisor;

                          int unit5 = unit / 5;
                          int remainder = unit % 5;

                          if (remainder == 4) {
                          // if the remainder is 4, then one of IV, IX, XL, XC, CD, CM
                          // i tells us if it is I, X, or C
                          // i + 1 tells us which pair: VX, LC, or DM.
                          // unit5 tells us if it is the first (V, L, or D) or second (X, C, or M)
                          out.append(DIGITS.charAt(i))
                          .append(DIGITS.charAt(i + 1 + unit5));
                          } else {
                          // add V, L, or D if needed
                          if (unit5 == 1) {
                          out.append(DIGITS.charAt(i + 1));
                          }

                          // add up to three I, X, or C
                          for (int j = 0; j < remainder; j++) {
                          out.append(DIGITS.charAt(i));
                          }
                          }

                          divisor /= 10;
                          }

                          return out.toString();
                          }

                          }


                          Your professor also might bar StringBuilder, although he shouldn't. Mixing calculation and output the way that your program does is a code smell. StringBuilder should be taught before System.out.print in my opinion. That said, the algorithm doesn't change if you replace the append calls with System.out.print.



                          The initial check if there is no remainder was unnecessary. The else case will handle it correctly. So I removed it.



                          I used a for loop as shorter and more readable than a while loop.



                          Moving the code to a separate method is simply better practice. Again, if your professor has not taught you that, that is a teaching failure. IMnsHO, method calls should be taught before if, else, and loops (for or while).



                          This code will throw an exception if given an input of 3900 or more (also probably for negative values). The exception will be confusing. It would be better to check for invalid values and throw a more sensible exception. However, your professor may not have taught exceptions yet.



                          I made DIGITS a class constant because it never changes throughout the program. You can make it a method variable if your professor hasn't taught class constants yet.






                          share|improve this answer












                          Control structure formatting




                                      }
                          // Checks 4
                          else if (onesDigit == 4) {
                          System.out.print (one + five);



                          Please don't put comments in between curly braces and their associated control statements.



                                      }
                          else if (onesDigit == 4) {
                          // Checks 4
                          System.out.print (one + five);


                          Just treat them as one unit. The problem with separating them is that it looks like the structure is over, because Java (and other C-style languages) use the same symbol for ending the control structure as ending the current piece of the structure.



                          I would actually write it as



                                     } else if (onesDigit == 4) {


                          Which is also the Java standard. But that's not as important as not adding more stuff between.



                          Comments



                          Most of your comments just restate what the code does. That makes sense in class, as the professor is telling you what the code does to help you learn. And since you may not understand what the code does, those kind of comments can make your programs easier to grade. But outside class, we expect programmers to be able to read code. Comments should not tell us what the code does, but why it is doing it.



                          Declarations



                          It is also the standard in Java to declare variables as late as possible, often at initialization. Your method looks more like old C code with all the declarations at the beginning.




                          try-with-resources



                          This may be past what your assignment allowed, but the right way to use an Autocloseable resource like a Scanner is



                          try (Scanner input = new Scanner(System.in)) {
                          // do stuff with the Scanner
                          }


                          This will automatically close the Scanner when it is done, even if there is an exception. It's essentially the equivalent of adding



                          } finally {
                          if (input != null) {
                          input.close();
                          }


                          I would also avoid putting spaces between a method name and the opening parenthesis. E.g. Scanner(System.in) is a constructor call. I would put a space in if (.



                          Putting it together



                          Adding the method from this answer (albeit with code that I find more readable):



                          class RomanNumeral {

                          private static final String DIGITS = "IVXLCDM";

                          public static void main(String args) {
                          try (Scanner input = new Scanner(System.in)) {
                          System.out.println(toRomanNumeral(input.nextInt()));
                          }
                          }

                          private static String toRomanNumeral(int value) {
                          StringBuilder out = new StringBuilder();

                          int divisor = 1000;
                          for (int i = DIGITS.length() - 1; i >= 0; i -= 2) {
                          int unit = value / divisor;
                          value %= divisor;

                          int unit5 = unit / 5;
                          int remainder = unit % 5;

                          if (remainder == 4) {
                          // if the remainder is 4, then one of IV, IX, XL, XC, CD, CM
                          // i tells us if it is I, X, or C
                          // i + 1 tells us which pair: VX, LC, or DM.
                          // unit5 tells us if it is the first (V, L, or D) or second (X, C, or M)
                          out.append(DIGITS.charAt(i))
                          .append(DIGITS.charAt(i + 1 + unit5));
                          } else {
                          // add V, L, or D if needed
                          if (unit5 == 1) {
                          out.append(DIGITS.charAt(i + 1));
                          }

                          // add up to three I, X, or C
                          for (int j = 0; j < remainder; j++) {
                          out.append(DIGITS.charAt(i));
                          }
                          }

                          divisor /= 10;
                          }

                          return out.toString();
                          }

                          }


                          Your professor also might bar StringBuilder, although he shouldn't. Mixing calculation and output the way that your program does is a code smell. StringBuilder should be taught before System.out.print in my opinion. That said, the algorithm doesn't change if you replace the append calls with System.out.print.



                          The initial check if there is no remainder was unnecessary. The else case will handle it correctly. So I removed it.



                          I used a for loop as shorter and more readable than a while loop.



                          Moving the code to a separate method is simply better practice. Again, if your professor has not taught you that, that is a teaching failure. IMnsHO, method calls should be taught before if, else, and loops (for or while).



                          This code will throw an exception if given an input of 3900 or more (also probably for negative values). The exception will be confusing. It would be better to check for invalid values and throw a more sensible exception. However, your professor may not have taught exceptions yet.



                          I made DIGITS a class constant because it never changes throughout the program. You can make it a method variable if your professor hasn't taught class constants yet.







                          share|improve this answer












                          share|improve this answer



                          share|improve this answer










                          answered Dec 15 at 18:42









                          mdfst13

                          17.1k52155




                          17.1k52155






























                              draft saved

                              draft discarded




















































                              Thanks for contributing an answer to Code Review Stack Exchange!


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              Use MathJax to format equations. MathJax reference.


                              To learn more, see our tips on writing great answers.





                              Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


                              Please pay close attention to the following guidance:


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              To learn more, see our tips on writing great answers.




                              draft saved


                              draft discarded














                              StackExchange.ready(
                              function () {
                              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f205650%2finteger-to-roman-numerals-1-2999%23new-answer', 'question_page');
                              }
                              );

                              Post as a guest















                              Required, but never shown





















































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown

































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown







                              Popular posts from this blog

                              Сан-Квентин

                              Алькесар

                              Josef Freinademetz