• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

ToyCar Game in Java

 
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This was at a recent written test...
There is a toycar placed on a 5 by 5 board.We can give 5 commands to it ,
PLACE(X,Y,F) where x denotes X Axis,y denotes Y Axis and F denotes the direction to which its facing.
MOVE->Move will move the toycar one step in the direction where its facing
LEFT->Left will turn the toycar by 90 degrees to its left and face it to the new direction .Note:Left will not move the toycar, it will just change the direction
RIGHT->Right will turn the toycar by 90 degrees to its right and face it to the new direction .Note:Right will not move the toycar, it will just change the direction
REPORT->Report shall tell me the X Axis,YAxis and Direction of the toycar. like 0,0,NORTH

Note:You cannot move,left,right,report the toycar unless you place it.

Assume its a Prod level code, Solve it in Java only.
Below is my code and I haven't been selected, please tell me if there is better way to do it because my solution is correct but probably very basic.Thanks.

 
Rancher
Posts: 2759
32
Eclipse IDE Spring Tomcat Server
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Kaur,

Please wrap your code in code tags. This makes it easier for others to read your code. There is a code button at the top that helps you
 
lowercase baba
Posts: 13089
67
Chrome Java Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I spent about 2 seconds looking at your code, and stopped. If this is supposed to be "production level" code, then you have a made a MAJOR oversight.

How many comments do you see in your code above?

If someone came to me for a code review with code like this, I send them back to their desk and tell them to correct that before wasting any more of my time. I would never allow un-commented code to go into my production system.
 
Rancher
Posts: 3742
16
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
And adding the commments should hopefully highlight a few other problems, but in case they don't

1. Why do you create a new ToyCar in the place method ? You already have a Toycar (the one you are calling the place method on), there's no need to create another one.
2. Declare a variable to hold the size of the board, then replace all uses of the value 5 with this variable. This will make it a lot easier to change the size of your board when the customer suddenly decides they wanted it 10 * 10 and not 5 * 5.
3. Once you've added this variable - decide if it should be static or an instance variable.
4. Use case statements instead of if/else to make decisions based on the value of an Enum.
5. Error handling should be done outside the class. Throw an exception rather than printing an error.
 
Tanya Kaur
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I had actually put comments...when I Had written the code at the interview...
It was expected not to throw any exceptions....Toycar shall ignore when he is tried to move outside the box...

I take two comments:
Taking 5 by 5 as variable input
and not to re-initialize toycar in place mthod...Do you see any better way of doing it,,,?Because I hav been rejected because of the code written above...
Is there any better way(Prod level code for instance)
 
Bartender
Posts: 3323
86
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
To add to Fred's and Joanne's comments:

1. Your constructed object won't work unless you call place(..) even if you use the constructor that takes the placement location and direction as constructor arguments.
2. You are returning a private enum from a public method so any class outside of this class won't be able to see the returned value.
3. If you don't call place(..) your code throws NPE's which isn't very friendly.
4. There is no test class to prove your class works. Production code must be tested before it is released.
 
Tony Docherty
Bartender
Posts: 3323
86
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

and not to re-initialize toycar in place mthod.


You aren't re-initializing it you are creating a new instance.
 
Tanya Kaur
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I still didn't get it...

1.So Place method's ToyCar instance is correct to assign xAxis,yAxis and Direction to the toycar
2.Where am in returning enum from the method...All methods are returning void
3.Yes NPE missing, I should have added that
4.I thought whatever I am adding to public static void main method is kind of testing....Is there any other way??
Is there any other way instead of making private ToyCar and all constructors...??
 
Tony Docherty
Bartender
Posts: 3323
86
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

1.So Place method's ToyCar instance is correct to assign xAxis,yAxis and Direction to the toycar


The place(..) method is in an instance of ToyCar, it doesn't need to create an instance.

2.Where am in returning enum from the method...All methods are returning void


getDirection()

Also your place(..) and setDirection(..) methods have a parameter of type ToyCar.Direction which is private so you can't use these public methods from outside the class.

3.Yes NPE missing, I should have added that


No, NPE is what gets thrown when you try to access an object reference which is null. It's very rare to throw one yourself as it generally means there's been a coding error.

4.I thought whatever I am adding to public static void main method is kind of testing....Is there any other way??



Well it is sort of testing but your testing isn't exhaustive and in fact tries to move the car off the board and so the move doesn't work. Proper testing at the very least will test legal and illegal values at each boundary for every method and could test complex combinations of methods.

Google for testing java code or check out Test4J and JUnit
 
Tanya Kaur
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Still in doubt,

1.Instance created in public static void main is not known to place method...Suppose calling method is in another class...Also instance created in Toycar is just to call the place method...Please correct if it can be done better...
2.Yes NPE is my bad, I should have handled that.
3.I will search for Testing methods....

Thanks for prompt reponses..
 
Tony Docherty
Bartender
Posts: 3323
86
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
place() is an instance method. When you create an instance of ToyCar main() and then use it to call place() the code executed in place() is in the instance you have just created.

Get rid of the ToyCar instance created in place() and remove all references to it from your code or if it makes more sense to you at this stage replace all "toycar." with "this."
 
Tanya Kaur
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Is this ok...(minus the comments)??

 
Tony Docherty
Bartender
Posts: 3323
86
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Remember to highlight all the code before pressing the code button.

Is this ok...(minus the comments)??


Have you written tests to prove it does work?

Does the 3 arg constructor work without having to call the place() method?
 
reply
    Bookmark Topic Watch Topic
  • New Topic