Is this OOP Java design ok?

IamtheWalrus

I'm still not sure about the changes I've made, in terms of separation of concerns. My design now is as follows:

1. Main class
2. Translator class
3. Input, validator, convertor and display impls.

Main is as follows:

public static void main(String[] args) {
		
		Translator run = new Translator();
		
		run.setInput(new ConsoleInput());
		run.setValidator(new RegexValidator());
		run.setConverter(new DigitToTextConverter());
		run.setDisplay(new ConsoleDisplayer());
		
		run.translate();
	}

public class Translator {
	
	private IInput     	input;
	private IValidator	validator;
	private IConverter	converter;
	private IDisplayer 	display;

	public void translate(){
		
		do{
			// 1. Get user input
			String number = input.getInput();
			// 2. Validate input
			boolean isValid = validator.validate(number);
			String text = "";
			if(isValid){
				// 3. Parse input
				int intNumber = parseText(number);
				
				// 4. Convert input to English words
				text = converter.convert(intNumber);
			}
			// 5. Display words to user
			display.display(text);
		}while(true);
	}

But now the translator class is tightly coupled to the impls. Haven't I just pushed the issue into a Main class?. Is there anyway around this or am I overly complicating things?

Aswerty

No the translator class is not tightly coupled anymore since you can pass any implementation you want to it now by changing the setter call in the main method. For me though it is poor design since not calling one of the setters will cause a null object exception to be thrown at runtime. Using constructor injection avoids this and forces anyone using the class to specify the implementation on initialization of the translator class. Setters will then allow you to change the implementation but are not use to initially define the implementation.

At some point in your code you have to tightly bind the implementation, which in this case is in the main method. The objective is to allow the implementation to be changed with little to no effort even if the original implementation is used extensively across a code base.

IamtheWalrus

Aswerty wrote: »

No the translator class is not tightly coupled anymore since you can pass any implementation you want to it now by changing the setter call in the main method. For me though it is poor design since not calling one of the setters will cause a null object exception to be thrown at runtime. Using constructor injection avoids this and forces anyone using the class to specify the implementation on initialization of the translator class. Setters will then allow you to change the implementation but are not use to initially define the implementation.

At some point in your code you have to tightly bind the implementation, which in this case is in the main method. The objective is to allow the implementation to be changed with little to no effort even if the original implementation is used extensively across a code base.

I had noticed that the app doesn't run unless I had set the impls. I have amended the app so that if can create a Translator class with a no-arg constructor then call the setters OR call a instance variable argumented constructor.

public Translator() {

	}
	public Translator(IInput input, IValidator validator, IConverter converter,IDisplayer display) {
		this.input = input;
		this.validator = validator;
		this.converter = converter;
		this.display = display;
	}

EDIT: This still leaves it possible to cause an exception to be thrown if I call the no-arg constructor but fail to call the setters on for impls.

GreeBo

I have swapped out the display impl with one that prints 'Hi'. When the user enters a number the program prints Hi.

I'm confused at which part of the program you are suggesting the changes. I'm not asking for you to write the program (I'd honestly prefer to learn) but can you be a bit more specific about what problem you are prompting me to solve?

So the point of this exercise was to show you how much had to change to achieve this. it should have been possible by just changing the main/caller, but in your case you had to edit the NumericToText.translate() method. This required anyone using your code to have the source code....not good.

For your current question:

Take a look at the builder or factory pattern...your client is trying to construct a valid NumericToText object...once you get more than a couple of arguments (constructor especially) its a bit of a code smell and you should think about providing the user with another way to create your objects (without having 4x3x2 versions of your constructor for each possibility!)

IamtheWalrus

GreeBo wrote: »

once you get more than a couple of arguments (constructor especially) its a bit of a code smell and you should think about providing the user with another way to create your objects (without having 4x3x2 versions of your constructor for each possibility!)

I thought of using the Factor pattern but isn't that a bit of overkill for an assignment of this size? I'm a little wary of making the design more complex that it has to be.

Would adding a comment explaining my thinking and suggesting a Factory/Builder would be a better way in a real-world app?

GreeBo

IamtheWalrus wrote: »

I thought of using the Factor pattern but isn't that a bit of overkill for an assignment of this size? I'm a little wary of making the design more complex that it has to be.

Would adding a comment explaining my thinking and suggesting a Factory/Builder would be a better way in a real-world app?

Hard to define overkill.
If the requirement is to make it as good as possible (including multiple refactorings) then I would say using a Factory or Builder pattern is fine.
Even if thats not the case, you are really trying to implement a builder.
The client of your code can create a NumericToText object in 24 different ways, depending on the input, thats the builder or factory pattern.

Sure, you dont have to implement it that way but you have already seen the pitfalls of having multiple constructor params and/or setters.

Finally, Id argue that using a well defined and well sued pattern isnt really making it more complicated than it should be, if the pattern fits then thats exactly what it should be used for.

IamtheWalrus

GreeBo wrote: »

Finally, Id argue that using a well defined and well sued pattern isnt really making it more complicated than it should be, if the pattern fits then thats exactly what it should be used for.

Yes, fair points. I will read on more on Builder patterns.

IamtheWalrus

I have taken the advice I've been given about my converter method and have amended it. I think it's much cleaner. Can anyone give me an opinion on the quality of the design? The only issue is that it doesn't cater for zero which I'm not happy about.

private final static String[] units = {"","one","two","three","four",
		"five","six","seven","eight","nine","ten",
		"eleven","twelve","thirteen","fourteen","fifteen",
		"sixteen","seventeen","eighteen","nineteen"};
	
	private final static String[] tens = {"","","twenty","thirty","forty","fifty",
		"sixty","seventy","eighty","ninety"};
	
	private final static String[] hundreds = {"hundred","thousand","million"};

	public String translate(int x) {
		String str = "";
		
		if(x<20){
			str += units[x];
		}else if(x<100){
			str += tens[x/10] + " " +translate(x%10);
		}else if(x<1000){
			str += translate(x/100) + " " + hundreds[0] + ((x % 100 > 0) ? " and " : "") +translate(x%100);
		}else if(x<1000000){
			str += translate(x/1000) + " " + hundreds[1] + " " + translate(x%1000);
		}else{
			str += translate(x/1000000) + " " + hundreds[2] + " " + translate(x%1000000);
		}	
		return str.trim();
	}

GreeBo

IamtheWalrus wrote: »

I have taken the advice I've been given about my converter method and have amended it. I think it's much cleaner. Can anyone give me an opinion on the quality of the design? The only issue is that it doesn't cater for zero which I'm not happy about.

	private final static String[] units = {"","one","two","three","four",
		"five","six","seven","eight","nine","ten",
		"eleven","twelve","thirteen","fourteen","fifteen",
		"sixteen","seventeen","eighteen","nineteen"};
	
	private final static String[] tens = {"","","twenty","thirty","forty","fifty",
		"sixty","seventy","eighty","ninety"};
	
	private final static String[] hundreds = {"hundred","thousand","million"};

	public String translate(int x) {
		String str = "";
		
		if(x<20){
			str += units[x];
		}else if(x<100){
			str += tens[x/10] + " " +translate(x%10);
		}else if(x<1000){
			str += translate(x/100) + " " + hundreds[0] + ((x % 100 > 0) ? " and " : "") +translate(x%100);
		}else if(x<1000000){
			str += translate(x/1000) + " " + hundreds[1] + " " + translate(x%1000);
		}else{
			str += translate(x/1000000) + " " + hundreds[2] + " " + translate(x%1000000);
		}	
		return str.trim();
	}

what's the problem with zero?
just put "zero" into the first position of your array, instead of ""?

IamtheWalrus

GreeBo wrote: »

what's the problem with zero?
just put "zero" into the first position of your array, instead of ""?

I tried that. If I try to convert 20, it returns "twentyzero".

GreeBo

IamtheWalrus wrote: »

I tried that. If I try to convert 20, it returns "twentyzero".

translate(x%10)
You need to check the result of x%10 before calling translate again to handle this situation.
if the result is 0 then you are done.

IamtheWalrus

GreeBo wrote: »

translate(x%10)
You need to check the result of x%10 before calling translate again to handle this situation.
if the result is 0 then you are done.

Yea, I had that in my initial solution but I took it out because it made the code very bulky and complex looking. I may have no other choice.

GreeBo

GreeBo wrote: »

translate(x%10)
You need to check the result of x%10 before calling translate again to handle this situation.
if the result is 0 then you are done.

another, better option would be to move str out into a class variable (and change to a string builder or buffer) then just don't add zero if str already had some value.

Aswerty

The value returned from the function will only ever be an empty string with the input is 0. So just check if the return value is an empty string and if it is return "zero" else return the value.

Also I'd generally implement something like this using a StringBuilder since strings are immutable, just saves on memory usage. Not a big deal if you don't use it.

Aswerty

Ah no actually I think that has the same issue as GreeBo's suggestion that you just change the first array element.

GreeBo

Aswerty wrote: »

The value returned from the function will only ever be an empty string with the input is 0. So just check if the return value is an empty string and if it is return "zero" else return the value.

that will have the same problem, the second call to translate for 20 will return zero unless str is outside of the recursive part off the code (or passed around, which is not so nice)

IamtheWalrus

GreeBo wrote: »

then just don't add zero if str already had some value.

Can you elaborate on this suggestion? I have to be mindful that the method is recursive.

GreeBo

IamtheWalrus wrote: »

Can you elaborate on this suggestion? I have to be mindful that the method is recursive.

the problem with your recursive solution is that you have no view on the current value of str, outside of the local scope.
if you move str outside of the recursive method you can check it at anytime.

then, if at anytime you find yourself returning from the translate method and str had no value, just return "zero" as the value.

IamtheWalrus

GreeBo wrote: »

the problem with your recursive solution is that you have no view on the current value of str, outside of the local scope.
if you move str outside of the recursive method you can check it at anytime.

then, if at anytime you find yourself returning from the translate method and str had no value, just return "zero" as the value.

Moving the str object out of the translate method means I'll have to redesign the method. If I input 100, I get one hundredone.

I'm beginning to think handling zero is too much bother.

GreeBo

IamtheWalrus wrote: »

Moving the str object out of the translate method means I'll have to redesign the method. If I input 100, I get one hundredone.

I'm beginning to think handling zero is too much bother.

Why?
You just need to remove String str = "";?
Increasing its scope shouldnt have any impact to the logic or require you to redesign the method??

IamtheWalrus

GreeBo wrote: »

Why?
You just need to remove String str = "";?
Increasing its scope shouldnt have any impact to the logic or require you to redesign the method??

It has a big impact. Each time the method is called, a new str is created and when returned from a call to translate, appended to the original string. With moving the str outside, the str is constantly being appended to and never gets reset.

GreeBo

IamtheWalrus wrote: »

It has a big impact. Each time the method is called, a new str is created and when returned from a call to translate, appended to the original string. With moving the str outside, the str is constantly being appended to and never gets reset.

Why is a new String being created each call?
The whole point of moving its scope outside that method is that a new String isnt created each time, you just append the result to the class level StringBuilder.
(for the nerds on here, yes I know new Strings are being created each time!)

Remove
String str = "";
from within the translate() method altogether.
make it a class level StringBuilder and initialise it to "" in your constructor.

each call to translate() then just appends its result to the ongoing result, when you have nothing left to process, return (which should be after the first "if")

IamtheWalrus

GreeBo wrote: »

Why is a new String being created each call?
The whole point of moving its scope outside that method is that a new String isnt created each time

I meant that the original design depended on a new string being created each time. If this doesn't happen, the method fails. I have made amendments as follows but all but 2 tests are failing.

	private final static int TEN = 10;
	private final static int HUNDRED = 100;
	private final static int THOUSAND = 1000;
	private final static int MILLION = 1000000;
	private StringBuilder str = new StringBuilder("");

	public String convert(int num) {
		
		if(num<20){
			str.append(units[num]);
		}else if(num<HUNDRED){
			str.append(tens[num/TEN]).append(" ").append(convert(num%TEN));
		}else if(num<THOUSAND){
			str.append(convert(num/HUNDRED)).append(" ").append(hundreds[0]).append(((num % HUNDRED > 0) ?" and ":"") +convert(num%HUNDRED));
		}else if(num<MILLION){
			str.append(convert(num/THOUSAND)).append(" ").append(hundreds[1]).append(convert(num%THOUSAND));
		}else{
			str.append(convert(num/MILLION)).append(" ").append(hundreds[2]).append(convert(num%MILLION));
		}	
		
		return str.toString().trim();
	}

If I input 20, I get 'twenty twenty' back.

GreeBo

IamtheWalrus wrote: »

If I input 20, I get 'twenty twenty' back.

Thats because you are returning str each time, like I said you only want to return it when you have processed all of the input.

i.e. when you have finished processing

if(num<20){
str.append(units[num]);

To do this I would remove the processing part of that method to its own method and call that from another one.
Once its complete, return your StringBuffer

e.g.

private StringBuffer str= new StringBuffer("");

public String translate(int input) {
    if(input == 0)
        return ZERO;
    else {
        processInput(input);
        return str.toString();
   }
}

private void processInput(int input) {
   		if(num<20){
			str.append(units[num]);
		}else if(num<HUNDRED){
			str.append(tens[num/TEN]).append(" ").append(convert(num%TEN));
		}else if(num<THOUSAND){
			str.append(convert(num/HUNDRED)).append(" ").append(hundreds[0]).append(((num % HUNDRED > 0) ?" and ":"") +convert(num%HUNDRED));
		}else if(num<MILLION){
			str.append(convert(num/THOUSAND)).append(" ").append(hundreds[1]).append(convert(num%THOUSAND));
		}else{
			str.append(convert(num/MILLION)).append(" ").append(hundreds[2]).append(convert(num%MILLION));
		}	
}

IamtheWalrus

No improvement. When I input 20, I get 'twenty zero'.

I appreciate your help on this. I'll just keep debugging.

GreeBo

IamtheWalrus wrote: »

No improvement. When I input 20, I get 'twenty zero'.

I appreciate your help on this. I'll just keep debugging.

that is an improvement!

did you forget to remove "ZERO" from position 0 of your array?

IamtheWalrus

GreeBo wrote: »

that is an improvement!

did you forget to remove "ZERO" from position 0 of your array?

Nope, that zero is history.

Just to clarify, in processInput(), the recursive calls are to translate and not to processInput()?

GreeBo

IamtheWalrus wrote: »

Nope, that zero is history.

Just to clarify, in processInput(), the recursive calls are to translate and not to processInput()?

Nope!
processInput calls processInput...otherwise its not a recursive method...
if processInput calls translate() then you have the exact same problem...as Im sure you have realised.
You can only return the result when you are done, otherwise each loop will add its new result to the existing running result.

Someone got a case of the copy/paste there methinks...:cool:

IamtheWalrus

GreeBo wrote: »

Nope!
processInput calls processInput...otherwise its not a recursive method...
if processInput calls translate() then you have the exact same problem...as Im sure you have realised.
You can only return the result when you are done, otherwise each loop will add its new result to the existing running result.

Someone got a case of the copy/paste there methinks...:cool:

I have made those amendments and now 20 gives 'twenty twenty'.

I have to amend processInput to return a string but it's still not working. Maybe this would be clearer in the morning. Want to get this done asap.

private StringBuilder str = new StringBuilder("");

	public String convert(int num) {
	    if(num==0)
	        return "zero";
	    else {
	        processInput(num);
	        return str.toString();
	   }

	}
	
	private String processInput(int num) {
   		if(num<20){
			str.append(units[num]);
		}else if(num<HUNDRED){
			str.append(tens[num/TEN]).append(" ").append(processInput(num%TEN));
		}else if(num<THOUSAND){
			str.append(processInput(num/HUNDRED)).append(" ").append(hundreds[0]).append(((num % HUNDRED > 0) ?" and ":"") +processInput(num%HUNDRED));
		}else if(num<MILLION){
			str.append(processInput(num/THOUSAND)).append(" ").append(hundreds[1]).append(processInput(num%THOUSAND));
		}else{
			str.append(processInput(num/MILLION)).append(" ").append(hundreds[2]).append(processInput(num%MILLION));
		}	
   		return str.toString();
	}

IamtheWalrus

For 20, it cannot work..

str.append(tens[num/TEN]).append(" ").append(processInput(num%TEN));

The first part will rightly give twenty, then the recursion will add twenty.