Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roman numerals solution #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions roman-numerals-solution/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
Hi,


## Implementation plan

* Create java main class with ability to read input from cmd pipe
* Implement convert algorithm
* Catch all possible exceptions
* Make some refac/add test/finally review

## User guide

1. Compile java source file:

G:>javac RomanNumeralsConvert.java

2. Run java application with cmd pipe:

G:>echo "1 12 103" | java RomanNumeralsConvert
I
XII
CIII

3. Add "-test" flag to run application in test mode:

G:>echo "1 12 103" | java RomanNumeralsConvert -test
1 => I passed.
12 => XII passed.
103 => CIII passed.
10char3 => 10char3, illegal argument error - is not valid digital number. passed.
10399911 => 10399911, range bound error - number should be in the range [1 - 3999]. passed.
167 changes: 167 additions & 0 deletions roman-numerals-solution/RomanNumeralsConvert.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class RomanNumeralsConvert {

public final static String onesArray[] = { "I", "II", "III", "IV", "V", "VI", "VII", "VIII", "IX" };
public final static String tensArray[] = { "X", "XX", "XXX", "XL", "L", "LX", "LXX", "LXXX", "XC" };
public final static String hundredsArray[] = { "C", "CC", "CCC", "CD", "D", "DC", "DCC", "DCCC", "CM" };

public final static String TEST_MOD = "-test";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move test data somewhere else.

public final static String ONE = "I";
public final static String TWELVE = "XII";
public final static String ONE_HUNDRED_AND_THREE = "CIII";

private static String[] numArray;
Copy link
Owner

@ZsoltFabok ZsoltFabok Sep 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You either solve this problem in an OOP fashion or functional, but this is approach is none of them. You use a static variable to pass around value. If you create two RomanNumeralsConvert this won't work.


public static void main(String[] args) {

if (args.length > 0) {
if (args.length > 1 || !TEST_MOD.equals(args[0])) {
System.out.println("Only one flag supports: - test");
} else {
runTest();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See later, but it would be better to have tests in a JUnit class.

}
} else {
readInput();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this style, but the methods should return something and the next method should use it as input.


validate();

convert();

printResult();
}

}

public static void readInput() {
Copy link
Owner

@ZsoltFabok ZsoltFabok Sep 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private, and this applies to the other methods.

try (BufferedReader in = new BufferedReader(new InputStreamReader(System.in));) {
String line;
while ((line = in.readLine()) != null) {
line = line.replace("\"", "").trim();
numArray = line.split("\\s+");
}
} catch (final IOException e) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you catch the exception here, the validate() method will still run

System.err.println("IOException reading System.in" + e);
}
}

public static void validate() {
for (int i = 0; i < numArray.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for(... : ...)

final String numStr = numArray[i];
boolean e = true;
for (int k = 0; k < numStr.length(); k++) {
if (Character.isDigit(numStr.charAt(k)) == false) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the == false, use if (! condition)

numArray[i] = numArray[i] + ", illegal argument error - is not valid digital number.";
e = false;
break;
}
}

if (e) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is hard to figure out what you are testing here.

final int num = Integer.parseInt(numStr);
if (num > 3999 || num < 1) {
numArray[i] = numArray[i] + ", range bound error - number should be in the range [1 - 3999].";
}
}
}
}

public static void convert() {
for (int i = 0; i < numArray.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, you should use collections, because there are way better functions to work with them than with arrays.

final String numStr = numArray[i];

if (numStr.contains("error")) {
continue;
}

String Roman = "";
int num = Integer.parseInt(numStr);

final int ones = num % 10;

num = (num - ones) / 10;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this and the following steps a bit more simpler?

final int tens = num % 10;

num = (num - tens) / 10;
final int hundreds = num % 10;

num = (num - hundreds) / 10;
for (int n = 0; n < num; n++) {
Roman += "M";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have "M" here, but the rest is at the top.

}

if (hundreds >= 1) {
Roman += hundredsArray[hundreds - 1];
}

if (tens >= 1) {
Roman += tensArray[tens - 1];
}

if (ones >= 1) {
Roman += onesArray[ones - 1];
}

numArray[i] = String.valueOf(Roman);
}
}

public static void printResult() {
for (int i = 0; i < numArray.length; i++) {
System.out.println(numArray[i]);
}

}

public static void runTest() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to write a JUnit test

numArray = new String[5];

numArray[0] = "1";
numArray[1] = "12";
numArray[2] = "103";

numArray[3] = "10char3";
numArray[4] = "10399911";

validate();

convert();

System.out.print("1 => " + numArray[0]);
if (ONE.equals(numArray[0])) {
System.out.println(" passed.");
} else {
System.out.println(" failed.");
}

System.out.print("12 => " + numArray[1]);
if (TWELVE.equals(numArray[1])) {
System.out.println(" passed.");
} else {
System.out.println(" failed.");
}

System.out.print("103 => " + numArray[2]);
if (ONE_HUNDRED_AND_THREE.equals(numArray[2])) {
System.out.println(" passed.");
} else {
System.out.println(" failed.");
}

System.out.print("10char3 => " + numArray[3]);
if ("10char3, illegal argument error - is not valid digital number.".equals(numArray[3])) {
System.out.println(" passed.");
} else {
System.out.println(" failed.");
}

System.out.print("10399911 => " + numArray[4]);
if ("10399911, range bound error - number should be in the range [1 - 3999].".equals(numArray[4])) {
System.out.println(" passed.");
} else {
System.out.println(" failed.");
}
}
}