Is this OOP Java design ok?

IamtheWalrus · 21-03-2013 12:43am #1

I have a puzzle that requires the following:

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

I am required to demonstrate the following:

Clear separation of concerns
Well defined objects / interfaces
Application of good OO design principles to solve the problem
No code duplication
Test Driven Development
Well refactored code
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 · 21-03-2013 6:02pm

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

Sorry, I'm no help

Aswerty · 21-03-2013 9:51pm

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 · 21-03-2013 10:03pm

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

IamtheWalrus · 21-03-2013 10:26pm

GreeBo wrote: »

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

I have used recursion for the converter...

IamtheWalrus · 21-03-2013 10:27pm

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 · 21-03-2013 10:29pm

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 · 21-03-2013 10:29pm

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 · 21-03-2013 10:35pm

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 · 21-03-2013 10:40pm

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

IamtheWalrus · 21-03-2013 10:41pm

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 · 21-03-2013 10:42pm

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 · 21-03-2013 10:55pm

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 · 21-03-2013 10:58pm

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 · 21-03-2013 11:15pm

+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 · 21-03-2013 11:24pm

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 · 21-03-2013 11:30pm

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 · 21-03-2013 11:33pm

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 · 21-03-2013 11:35pm

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 · 21-03-2013 11:36pm

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 · 21-03-2013 11:44pm

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 · 21-03-2013 11:56pm

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 · 21-03-2013 11:57pm

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 · 22-03-2013 12:01am

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 · 22-03-2013 12:05am

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 · 22-03-2013 12:08am

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 · 22-03-2013 12:11am

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 · 22-03-2013 12:23am

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 · 22-03-2013 12:42am

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 · 22-03-2013 1:06am

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 · 22-03-2013 12:06pm

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.

Is this OOP Java design ok?

Comments