Is this OOP Java design ok?

IamtheWalrus

I have a puzzle that requires the following:

1. Take a number as input.
2. Convert that number to its English version.
3. Display that number.

I am required to demonstrate the following:

1. Clear separation of concerns
2. Well defined objects / interfaces
3. Application of good OO design principles to solve the problem
4. No code duplication
5. Test Driven Development
6. Well refactored code
7. Well tested code

I have designed and written the program and would like people's opinions on my design taking the above 7 points into consideration.

Would someone be willing to have a look at my code and tell me if it's a good solution?

I have attached the archive.

Tar.Aldarion

I thought junit worked automatically in eclipse? Yet I get: The import junit cannot be resolved.

Sorry, I'm no help

Aswerty

I think your solution demonstrates the 7 points very well.

The only things I might look at is the Converter implementation seems like a fairly naive approach, tbh I don't know if there is a better way. Because of this the ConverterTest class requires a large number of tests. This would fall under point 6.

Also no class/method descriptions, though they're well named and easy to understand so depending on who your doing this for you may or may not need them. I suppose this comes under point 2.

GreeBo

Looks pretty good...have you considered using recursion for the Converter implementation?

IamtheWalrus

GreeBo wrote: »

Looks pretty good...have you considered using recursion for the Converter implementation?

I have used recursion for the converter...

IamtheWalrus

Aswerty wrote: »

The only things I might look at is the Converter implementation seems like a fairly naive approach, tbh I don't know if there is a better way.

I would like to change the converter method. Have you any pointers to an alternative method?

ChRoMe

IamtheWalrus wrote: »

I would like to change the converter method. Have you any pointers to an alternative method?

Is this is a test for a job application?

Molly

He's using recursion (a little bit)but could definitely do a better job of it. The lack of method/class commenting was annoying, just screams of laziness. Some style things I found to be annoying, final before private (maybe I'm just used to sonar giving out to me about it) and whatever code template you were using was a pain also

Not sure why you're doing this.

input 	  = new UserInput();
		validator = new InputValidator();
		converter = new Converter();
		display   = new DisplayResult();
		
		this.setInput(input);
		this.setValidator(validator);
		this.setConverter(converter);
		this.setDisplay(display);

your setters are only doing

public void setInput(IUserInput input) {
		this.input = input;
	}

	public void setValidator(IValidator validator) {
		this.validator = validator;
	}

	public void setConverter(IConverter converter) {
		this.converter = converter;
	}

	public void setDisplay(IDisplayResult display) {
		this.display = display;
	}

You could have just done

this.validator = new Validator();

The comment '//setters' looks like a comment for the sake of a comment

This looks like a grad interview assignment?

IamtheWalrus

ChRoMe wrote: »

Is this is a test for a job application?

This is an assignment for a job. It's not ready to be submitted. I have to format and comment all classes. It's more the approach I'm asking for opinions on.

Tar.Aldarion

Would like to see it when it's finished also!

IamtheWalrus

Molly wrote: »

You could have just done

this.validator = new Validator();

Isn't it standard to have setters for all private instance variables?

I have made the changes you suggested.

IamtheWalrus

Molly wrote: »

He's using recursion (a little bit)but could definitely do a better job of it.

I'd really appreciate if you suggested a better approach. I've spent a good while at it but cannot find a better way.

Molly

I might want to retract the word definitely, but it just seems like you could do a better job. Don't really have the time/inclination to think about it right now.

Regarding the setters, no it's not. I work with Spring a lot and make use of IoC and annotations to do what you've done. That's not to say you should do that for this project. I think it might be slightly overkill...

If you're insistent on using the setters make it something like

this.setValidator(new Validator());

What you're doing now is basically setting the variable to a new instance of Validator, then calling a setter to set the variable to be equal to itself. There's no need for that.

GreeBo

Molly wrote: »

He's using recursion (a little bit)but could definitely do a better job of it.

That was kinda my point!
:pac:

GreeBo

+1 on what Molly is saying

Also, why do you have these objects as members of the class....from what I can see they are only used within translate(), so why not keep them to that scope (and make translate a static/utility class)?
(thats a question by the way, not a recommendation!)


/edit
I had a bunch of crap there about what I would do, but I dont want to feed you an answer.
However I'll pose another question to maybe prompt you to think about it.
You have coded to Interfaces....why?
What benefit are you getting from this *with your current* implementation of NumericToText class?

If I gave you a class right now that implements IUserInput by reading from a file...what do you need to do to switch it in?
Or
Imagine your main() method is actually a client of your code that is just using a jar'd version of your class and it wants to use your handy class...but it wants to send the output to an email and not print to screen.

Right now the client cant do that without editing your code...so what the point in you coding to interfaces if you cant easily change the implementation? (forget about IOC here)

IamtheWalrus

GreeBo wrote: »

However I'll pose another question to maybe prompt you to think about it.
You have coded to Interfaces....why?
What benefit are you getting from this *with your current* implementation of NumericToText class?

If I gave you a class right now that implements IUserInput by reading from a file...what do you need to do to switch it in?
Or
Imagine your main() method is actually a client of your code that is just using a jar'd version of your class and it wants to use your handy class...but it wants to send the output to an email and not print to screen.

Right now the client cant do that without editing your code...so what the point in you coding to interfaces if you cant easily change the implementation? (forget about IOC here)

Agreed. I had intended on returning to that bit. I also noticed that if I have the user input from the console, yet swap out the display class with a file writer, the user would never see the results.. S/he would have to open the file and look.

IamtheWalrus

GreeBo wrote: »

so what the point in you coding to interfaces if you cant easily change the implementation? (forget about IOC here)

I still want to demonstrate coding to interfaces so I'll have to find a way to change the implementations.

GreeBo

IamtheWalrus wrote: »

Agreed. I had intended on returning to that bit. I also noticed that if I have the user input from the console, yet swap out the display class with a file writer, the user would never see the results.. S/he would have to open the file and look.

This is kinda my point....dont think of the user as the person typing in the number, think of the user as the person who is calling your class.
They are the ones who have real, end users to worry about.

IMO It should be up to them to decide what implementations they want to provide, or just use your given ones. So if they use a File based implementation of IDisplay, then thats totally fine.

But right now your class is actually tightly coupled to your current implementations ( despite you coding to interfaces) as you dont expose anyway for any caller to use different implementations, they need to edit and recompile your code.

IamtheWalrus

GreeBo wrote: »

+1 on what Molly is saying

Also, why do you have these objects as members of the class....from what I can see they are only used within translate(), so why not keep them to that scope (and make translate a static/utility class)?
(thats a question by the way, not a recommendation!)

That makes sense. The class itself doesn't use those variables. Only translate does.

GreeBo

IamtheWalrus wrote: »

I still want to demonstrate coding to interfaces so I'll have to find a way to change the implementations.

Bingo, there are design patterns that will do just this ( I can think of two specifically ) or you can do it a less nice but just as functional way; but either way, as a pointer...its not you who is changing the implementations, its the caller...you just have to allow them to do it.

FYI Im not just giving you (my) solution as the point of this forum is not to hand out things to people, especially if its for a job interview/exam/etc.

IamtheWalrus

GreeBo wrote: »

Bingo, there are design patterns that will do just this ( I can think of two specifically ) or you can do it a less nice but just as functional way; but either way, as a pointer...its not you who is changing the implementations, its the caller...you just have to allow them to do it.

FYI Im not just giving you (my) solution as the point of this forum is not to hand out things to people, especially if its for a job interview/exam/etc.

I can see your point - the coding to interfaces is absolutely redundant in this design. It doesn't allow the swapping out of any classes. The classes are all tied to the NumberToText class.

IamtheWalrus

I need to find a non-programmatic method of first setting the interface implementations before running the program. I'm so used to injecting the impls in via Spring. I've never needed to do it any other way.

I'm stumped for now. I'll have to have a think about it.

GreeBo

Technically NumberToText class is tied to those specific implementations but I think you see the point.

Its fine (well, fine-ish) for the caller (in this case your main method) to be tied to specific implementations, but the class thats actually doing the work is currently tied, making it non portable.

Tom1991

Hi,sorry to interrupt but im a first year studying programming.Just was wondering in the test class you have tests to see if X number returns X as far as 201.
Is there no way where you could have 3 arrays and check the number against them like 201 = two hundred and one
string[] small = one, two three etc
string[] middle = ten, twenty thirty etc
string[] larger = hundred, thousand,million
then in a for loop
or use if statements to say if its 3 digits long then at position i = string middle[0
and so forth.
Im sorry this isnt relevant to your problem and if this is infringing please feel free to delete the post and finally im sorry that this isnt clearer but im playin the noob card here.

GreeBo

IamtheWalrus wrote: »

I need to find a non-programmatic method of first setting the interface implementations before running the program. I'm so used to injecting the impls in via Spring. I've never needed to do it any other way.

I'm stumped for now. I'll have to have a think about it.

In Spring the caller decides what implementations to inject at runtime (either through annotations, or bean definitions etc)
Since you are not using Spring in this case you still need to allow the caller to specify what implementation they want to use (or, using one of the design patterns maybe, what specific implementations they want to override, leaving the rest at the defaults?)


First think about the basic approach, without using any design pattern.
If I gave you a Person class that had a firstname and a surname members, who would you use that class?

Would it be at all helpful to you if that class had a toString method that say returned "Hello "+this.firstname+" "+this.surname compared to say a toString method that first always did

this.firstname = "My implementation of first name"
this.surname = "my implementation of surname"
return "Hello "+this.firstname+" "+this.surname



Whats the purpose of your public setters?

IamtheWalrus

GreeBo wrote: »

Technically NumberToText class is tied to those specific implementations but I think you see the point.

Its fine (well, fine-ish) for the caller (in this case your main method) to be tied to specific implementations, but the class thats actually doing the work is currently tied, making it non portable.

I think I should remove the main method from NumberToText. Create a new class to house the main method (class called Main say) and use this class to set the implementations of the interfaces.. This means the swapping out of impls is separate from the actual app and I'm just using the new Main class as a way tio inject in the impls.

What do you think of that?

IamtheWalrus

GreeBo wrote: »

Whats the purpose of your public setters?

You're talking about method overriding here. I can use the default method or use my own.

GreeBo

IamtheWalrus wrote: »

I think I should remove the main method from NumberToText. Create a new class to house the main method (class called Main say) and use this class to set the implementations of the interfaces.. This means the swapping out of impls is separate from the actual app and I'm just using the new Main class as a way tio inject in the impls.

What do you think of that?

Its often easier to separate out the main method alright, helps you to realise that you might not always have access to this code, so you think about things now, rather than later, when its an issue!

IamtheWalrus wrote: »

You're talking about method overriding here. I can use the default method or use my own.

Nope, just ask yourself what use a sayHello() method on a Person class would be if it sets its own firstname and surname values everytime you call it on your own instance of a Person (which you have set the firstname and surname that you want)
This is essentially the same problem that you have with your code today.

To illustrate it, once you have created your new main class, also create a new implementation of Display that prints "hi" first or something...then using the same main method, call NumericToText first with the current Display implementation and then with the new "hi" version. (also your implementation classes should tell you something about the implementation, e.g. ConsoleDisplay implements IDisplay
tells me something about it, just
Display implements IDisplay
tells me nothing, I have to read the source code.java docs.)

IamtheWalrus

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?

IamtheWalrus

What I think you are trying to suggest is that my program should have default behaviour incase the user doesn't specifically set the instance variables.

Then only way I can think of this is via abstract classes, with default behaviours. Would it be best ton inject the impls via constructor setting?

Apologies for the silly questions. I've had a rough day and I'm not thinking straight.

Aswerty

I'd agree that pushing the translator and parseText functionality into a new class should be done. At this point you'll notice that the translator method is calling the input and display functionality which aren't really its responsibility. So you should be able to push input/display method calls into the main function.

With regards to the loose coupling of functionality, I think we are making things a bit more complicated than they need be. Some constructor injection for saying how conversion and validation for the translate method should be implemented is a good approach. For the input and display functionality I think a simple IFoo = new Foo() approach is as far as you need to go. So GreeBo's friendly input would mean changing IUserInput input = new Input() to IUserInput input = new FriendlyInput() in the code. For me setting the specific implementation should be done in the main method but if your fussed you can create classes for managing the implementation specifically.

Also with regards to Setters. Setters are there for encapsulation purposes so that the state of an object can be changed during it's life time without fooling around with a classes variables from an external class. So if you want your implemenation to be changeable after instantiation you should have setters otherwise your constructor should handle setting the implementation. I think those who pointed out your current setter/implementation/interface setup is lacking are dead on.

What I think you are trying to suggest is that my program should have default behaviour incase the user doesn't specifically set the instance variables.

Then only way I can think of this is via abstract classes, with default behaviours. Would it be best ton inject the impls via constructor setting?

Apologies for the silly questions. I've had a rough day and I'm not thinking straight.

I think your interface approach is sufficient. You could use abstract classes but I don't see any major reason to change your design. The program doesn't need default behaviour it just needs at least one implementation and be designed that changing that implementation should involved changing one line of code per class being changed.

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??