Is this OOP Java design ok?

GreeBo

IamtheWalrus wrote: »

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();
	}

Again you have the same problem.
*Why* are you making processInput return a String?!

GreeBo

IamtheWalrus wrote: »

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.

the recursion will only add "Twenty" if you make it.
The whole point of the refactor was to NOT return anything from processInput.
It doesnt need to return anything, its "returning" its result by appending to the StringBuffer.
Appending to the StringBuffer AND returning something is duplicating the result and thus giving you the result twice in the output.

IamtheWalrus

GreeBo wrote: »

Again you have the same problem.
*Why* are you making processInput return a String?!

I have it returning a StringBuilder. It needs to reurn something to be appended to the recursive StringBuilder.

Maybe I'm missing something..

IamtheWalrus

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

the

.append(processInput(num%TEN));

part needs processInput to return something to enable the appending.

GreeBo

IamtheWalrus wrote: »

I have it returning a StringBuilder. It needs to reurn something to be appended to the recursive StringBuilder.

Maybe I'm missing something..

yep

Why does it need to return something?
Every call is *already* adding its result to the same StringBuilder...str is at the class level, so each call (recursive or otherwise) is appending to the same object.

Using 20 as an example

right now you are saying

FIRST CALL:
its >=20 and <100 so append TWENTY to str and also append the result of calling myself again

SECOND CALL:
Its < 19 so append "" to str
now return the current value of str (TWENTY+"") to what ever called me

so now FIRST CALL is appending (TWENTY) + (TWENTY+"")
and this you get TWENTY TWENTY

if however you just stop returning anything

FIRST CALL is appending TWENTY to "" so you get what you actually want

GreeBo

IamtheWalrus wrote: »

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

the

.append(processInput(num%TEN));

part needs processInput to return something to enable the appending.

Nope it doesnt, the processInput is already doing the appending internally, each recursive call takes care of updating the overall result itself.
Returning it means it gets done by the call and the thing that called the call = duplication.
so instead of calling append(processInput())
just call processInput()

IamtheWalrus

GreeBo wrote: »

so instead of calling append(processInput())
just call processInput()

I dont get it. There I've said it.

The line

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

Now becomes:

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

How do I get the first bit to tag onto the second bit?

GreeBo

IamtheWalrus wrote: »

I dont get it. There I've said it.

The line

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

Now becomes:

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

How do I get the first bit to tag onto the second bit?

You don't append it!
after you have added in the spacer string you are done.
New line.
call processinput( num%10)
and IT looks after appending its result in the exact same way it added tens[num/10]+" "

IamtheWalrus

I think you mean this:

private void processInput(int num) {
   		if(num<20){
			str.append(units[num]);
		}else if(num<HUNDRED){
			str.append(tens[num/TEN]).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));
			processInput(num/HUNDRED);
			str.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));
			processInput(num/THOUSAND);
			str.append(" ").append(hundreds[1]);
			processInput(num%THOUSAND);
		}else{
			//str.append(processInput(num/MILLION)).append(" ").append(hundreds[2]).append(processInput(num%MILLION));
			processInput(num/MILLION);
			str.append(" ").append(hundreds[2]);
			processInput(num%MILLION);
		}	
	}

GreeBo

IamtheWalrus wrote: »

I think you mean this:

	private void processInput(int num) {
   		if(num<20){
			str.append(units[num]);
		}else if(num<HUNDRED){
			str.append(tens[num/TEN]).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));
			processInput(num/HUNDRED);
			str.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));
			processInput(num/THOUSAND);
			str.append(" ").append(hundreds[1]);
			processInput(num%THOUSAND);
		}else{
			//str.append(processInput(num/MILLION)).append(" ").append(hundreds[2]).append(processInput(num%MILLION));
			processInput(num/MILLION);
			str.append(" ").append(hundreds[2]);
			processInput(num%MILLION);
		}	
	}

so does it work!?

IamtheWalrus

I had to do some trimming and add in an extra space but yes, all tests are passing.

I really appreciate your help.

The integration tests are failing with incorrect output but I can debug those.

EDIT: all tests are passing.

I have found other versions of this method here:
http://stackoverflow.com/questions/794663/net-convert-number-to-string-representation-1-to-one-2-to-two-etc

I am careful not to just copy any version found on the web. The person marking this might be aware of web versions. Do you think my new version is as impressive? Is it a clever method? I want to impress.

GreeBo

IamtheWalrus wrote: »

I had to do some trimming and add in an extra space but yes, all tests are passing.

I really appreciate your help.

The integration tests are failing with incorrect output but I can debug those.

EDIT: all tests are passing.

I have found other versions of this method here:
http://stackoverflow.com/questions/794663/net-convert-number-to-string-representation-1-to-one-2-to-two-etc

I am careful not to just copy any version found on the web. The person marking this might be aware of web versions. Do you think my new version is as impressive? Is it a clever method? I want to impress.

hmm, it's different now alright but to be honest it was identical before and I reckon you would have been done for copying it verbatim.

can you talk through why you did each line now?
it might be an idea to create some other implementations of the interface and site how you can now run both from your main class sequentially.

IamtheWalrus

GreeBo wrote: »

hmm, it's different now alright but to be honest it was identical before and I reckon you would have been done for copying it verbatim.

can you talk through why you did each line now?
it might be an idea to create some other implementations of the interface and site how you can now run both from your main class sequentially.

I think the fact it is different to any web version might be the most important point. At least it shows (semi!)-independent thought. And hopefully the other parts display good development. Ill thoroughly comment the good to highlight I know what I'm doing.

IamtheWalrus

I thought my solution was perfect but upon further testing, it's not correct. One occasions the 'and' is not being included and the space between the words is either absent or duplicated. Ruddy frustrating considering I was ready to submit it.

Molly

What do you think will happen if your interviewer comes across this?

IamtheWalrus

Molly wrote: »

What do you think will happen if your interviewer comes across this?

Ill fail but it's better than no solution or a solution that has less chance of passing.

IamtheWalrus

I'm almost finished now.

Last issue is 100,100 is returning 'one hundred thousand one hundred'.

Does anyone think it should be 'one hundred thousand and one hundred'?

Aswerty

IamtheWalrus wrote: »

I'm almost finished now.

Last issue is 100,100 is returning 'one hundred thousand one hundred'.

Does anyone think it should be 'one hundred thousand and one hundred'?

One thousand one hundred sounds right.