Integer to Roman Numerals (1 - 2999)
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
add a comment |
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
add a comment |
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
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
java beginner homework roman-numerals
edited Oct 16 at 6:04
200_success
128k15149412
128k15149412
asked Oct 16 at 5:40
Henry So
61
61
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
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;
}
add a comment |
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.
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
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;
}
add a comment |
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;
}
add a comment |
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;
}
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;
}
answered Oct 16 at 11:54
Holger
20613
20613
add a comment |
add a comment |
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.
add a comment |
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.
add a comment |
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.
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.
answered Dec 15 at 18:42
mdfst13
17.1k52155
17.1k52155
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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