aspose file tools*
The moose likes Beginning Java and the fly likes Rate my code Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Spring in Action this week in the Spring forum!
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "Rate my code" Watch "Rate my code" New topic
Author

Rate my code

Mick Jones
Greenhorn

Joined: May 21, 2009
Posts: 7
Hi everyone

I am new to Java and have been reading the Head First Java book. I think the book is brilliant but the only thing it lacks is the amount of "Now you have learnt this, try building or developing a...". So, I decided to create a project to help me put into practice what I have been taught. I have tried to create a vending machine (nowhere near finished) but just wondered if you guys could take a look and tell me if it's any good (in it's current state), and that I haven't made any stupid design errors or any "why the heck did you put that there".

I know it's basic but I am learning (so no laughing!), so actually quite happy with this

VendingMachine.java



Snack.java



Thanks a lot!


Currently learning Java
Fred Hamilton
Ranch Hand

Joined: May 13, 2009
Posts: 679
The code itself seems pretty neat and clean, but I'm more interested in what your thinking was when you made decisions of your data structure. Why use a hash map? Not saying it's a bad idea, but i'm interested in your reasons for designing it the way you did.

One other thing that would help would be an introduction or high level overview, not too technical, of what your program does. or how it simulates a real life vending machine. I could probably figure it out, but it's better to hear it from you
Matthew Cox
Greenhorn

Joined: May 25, 2009
Posts: 29
Fred Hamilton wrote:The code itself seems pretty neat and clean, but I'm more interested in what your thinking was when you made decisions of your data structure. Why use a hash map? Not saying it's a bad idea, but i'm interested in your reasons for designing it the way you did.

One other thing that would help would be an introduction or high level overview, not too technical, of what your program does. or how it simulates a real life vending machine. I could probably figure it out, but it's better to hear it from you



I would definitely have to agree. The code looks clean but some items just from a quick glance look unncessary as almost if you saw it in the example of a book and have just been doing it since. Not sure though.

The most important piece to appraising code is what the coder's intent is ... ie if you are trying to code a car but don't implement the wheels or the hood and their intrinsic funtionality ... okay, code is no good. =P

Let's hear what YOU think your code is supposed to do.

P.s

Things like "reasons for choosing the data structure (the hashmap)" or "why these classes are related" etc...
Michael Dunn
Ranch Hand

Joined: Jun 09, 2003
Posts: 4632
vending machines have stock, snacks don't.

[edit]
should be a snack doesn't
Fred Hamilton
Ranch Hand

Joined: May 13, 2009
Posts: 679
Michael Dunn wrote:vending machines have stock, snacks don't.

[edit]
should be a snack doesn't


maybe stock is just a bad choice of words? cause here stock refers to the supply (quantity on hand) of a particular snack.
Michael Dunn
Ranch Hand

Joined: Jun 09, 2003
Posts: 4632
> cause here stock refers to the supply (quantity on hand) of a particular snack.

yes, and that's part of the vending machine's job.

a snack doesn't hnow how many brothers and sisters it has
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11477
    
  94

Hi,

As previously stated, your code looks clean enough. But since you have asked for comments, I am going to be pickier than I probably would be if you worked for me. Please do not take this the wrong way - I can read your code, and it seems to do what you have designed it to do, so it is certainly a good start.

Package names (e.g. VendingMachine) by convention start with a lower case letter, and are more commonly a single word used to describe the types of classes we expect to find in that package. (Having said that, I often fail to keep to a single word myself).

I would not have expected to have both the VendingMachine class and the Snack class in the same package. They are only related in that sometimes you get Snacks in VendingMachines. But it is quite plausible to have a VendingMachine that does not dispense Snacks. Likewise it is possible to purchase Snacks from somewhere other than a VendingMachine. Given that, I would possibly consider having one set of packages for "vendors", which could include VendingMachine, Store, Vendor, etc. I might then have another package for "salable" (terrible name - I would want to think about that) that might include Snacks, Drinks, Cigarettes etc. It doesn't really matter if you never create all the other classes - just having the packages there for them in the future will make your application much easier to understand.

Now having decided that there are at least 2 different package groups, I would probably try and work out what is conceptually similar about them all. For example, for all the salable items we might want to be able to setPrice, reportPrice, getName, and so on. Since this functionality is likely to exist for all our salable items, I would probably put the definitions into an Interface.

You only had an updateStock() method, which performs a decrement. Personally I feel the method name you chose does not accurately reflect what the method does. For all my nit-picking, this is probably my biggest concern - so you can see that your coding attempt is off to a good start! See also the earlier comments about whether the stock count belongs with the Snack object or not.

While on the subject of your updateStock() method - did you intend that to be a public method? Given that it only decrements stock counts, I would have expected it to have a little less exposure. But I don't know what your future plans are, so this may not be an issue.

And still in the updateStock() method - have you considered the decrement operator? I would prefer to read stock-- than stock = stock - 1

I assume that at some point in the future you will be using the snackMap object that you are passing into the buySnack() method. This concerns me a little, as I suspect that you will end up with the Snack class modifying an instance method of the VendingMachine class. Normally I would recommend against this, but without knowing your plans, it is a little hard to comment.

You have hard-coded the magic number of 1 into your buySnack() method. Magic numbers are generally considered to be a bad idea - it is better to have a constant somewhere that describes what that number is for. For example private static final int MINIMUM_STOCK = 1. I would even consider making this magic number a variable so that we can change to other vendor model at a later date - one that doesn't have a minimum of 1. Two examples I can think of are both online-retail: if you were selling books, you might decide that the Harry Potter books can have a minimum value of -100, as you are getting boxes of 100 books a day, so if you sell 20 more than you actually have on stock you will catch up the next day. The same thing happens with airlines - they always overbook seats. Depending on the flight, they vary how many seats they overbook. For an international flight that they only service once a day, they might only allow 5 seats to be overbooked. For the 5am flight on a route that they service every half hour they might allow 100 seats to be overbooked.

So - had enough of my nit-picking yet?

If you would really like to work through some programming projects with set goals where you will be nit-picked by experts, you might want to consider the JavaRanch Cattle Drive.

Good luck with your programming, and please continue to seek code reviews no matter what you do. Even after 20 years of programming, I still get code reviews when I can, and I still learn things. XP (extreme programming) even has a core concept of pair programming in which the "pair" is performing a continuous review of the code as it is being written - and there is plenty of evidence that this results in better code.


The Sun Certified Java Developer Exam with J2SE 5: paper version from Amazon, PDF from Apress, Online reference: Books 24x7 Personal blog
Fred Hamilton
Ranch Hand

Joined: May 13, 2009
Posts: 679
Michael Dunn wrote:> cause here stock refers to the supply (quantity on hand) of a particular snack.

yes, and that's part of the vending machine's job.

a snack doesn't hnow how many brothers and sisters it has


ok now I see what you mean. good point, however, and I don't mean to quibble, but here an instance of snack would be Oh Henry, not a particular Oh Henry bar. In which case it seems to me that we might still be ok.

And you are right about the machine itself keeping track, this is just the way it does so. it's really just a category with several types of snack, each type is an instance with a quantity

Do you agree?

regards.
Michael Dunn
Ranch Hand

Joined: Jun 09, 2003
Posts: 4632
(2nd attempt at post - mods please delete this if duplicated)

> it's really just a category with several types of snack, each type is an instance with a quantity

I'd probably create an InventoryItem (or VendingMachineItem) class, which would have a snack and a quantity.
the VendingMachine would then contain a collection of InventoryItems.
(you could have snack extend InventoryItem)
Fred Hamilton
Ranch Hand

Joined: May 13, 2009
Posts: 679
Michael Dunn wrote:(2nd attempt at post - mods please delete this if duplicated)

> it's really just a category with several types of snack, each type is an instance with a quantity

I'd probably create an InventoryItem (or VendingMachineItem) class, which would have a snack and a quantity.
the VendingMachine would then contain a collection of InventoryItems.
(you could have snack extend InventoryItem)


cool, I think we agree, cause as I see it, that's exactly what would happen if you renamed the Snack class to InventoryItem

cheers.

on second thought, given that he is only tracking three things, snack type, quantity, and price, I probably wouldn't bother with creating a subclass to hold only one variable, a description.
Campbell Ritchie
Sheriff

Joined: Oct 13, 2005
Posts: 39393
    
  28
Andrew Monkhouse wrote: . . . I would not have expected to have both the VendingMachine class and the Snack class in the same package. . . . one set of packages for "vendors", which could include VendingMachine, Store, Vendor, etc. I might then have another package for "salable" (terrible name - I would want to think about that)
How about machine and product for the package names?
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11477
    
  94

Campbell Ritchie wrote:
Andrew Monkhouse wrote: . . . I would not have expected to have both the VendingMachine class and the Snack class in the same package. . . . one set of packages for "vendors", which could include VendingMachine, Store, Vendor, etc. I might then have another package for "salable" (terrible name - I would want to think about that)
How about machine and product for the package names?

I like product - that is a good description of the sort of classes I might expect to find there.

But "machine" I am not so sure about - I suggested that we could have VendingMachine, Store and Vendor as examples of classes that might sell products. Going from my later example, we might also have Website. Most of these wouldnt fit my definition of machine.
Mick Jones
Greenhorn

Joined: May 21, 2009
Posts: 7
Wow thanks for the feedback.. wasn't expecting such constructive and positive stuff. Sorry for the long replies below

To answer a few questons:

------------------------
Reason for using HashMap
------------------------

I will start by telling you what I am trying to do...

I need the 'snacks'(items) to live in set locations in the machine i.e. "A1", "A2" or E6 etc (A being the row and the number being the collumn in the machine). These locations will change very rarely.

So...

I looked into the various options (saw the collections stuff in the K&B book), firstly I looked at ArrayList but when I tried to use it I quickly realised I couldn't access certain locations of the array by using user input i.e. "A1" I would need to say "1" and then it wouldn't know which item in the array the user wanted, so i guessed I needed some sort of key/value pair i.e. "Give me the info for the snack at location A1". I guess looking back I could have used an ArrayList and program the array numbers to buttons but that just seemed overkill.

The answer in short is I don't know enough about Java and what options are available to me, hopefully this will come from experience and some of you guys could slightly tweek parts of my code to give me some pointers of what you would have done (i.e. a better way)

--------------------------------------------------
snackMap object passing into the buySnack() method
--------------------------------------------------
Again, I only did this because I needed to pass a reference to the current snack into the buy method - just me being a beginner again.

Any ideas how to make this more safe / cleaner?

------------------------
updateStock()
------------------------
I agree, the naming is pretty rubbish, I had a brain freeze moment - that's actually the 5th name it's had

------------------------
General
------------------------
Thanks for the tips on package names and the decrement (variable--). I knew these but forgot

Again, thanks for everyones help!

p.s. I can't seem to reply to posts within FireFox, it asks me to login everytime. When I login it asks me to login again . Appologies if this is a well documented problem!
Campbell Ritchie
Sheriff

Joined: Oct 13, 2005
Posts: 39393
    
  28
When you log in, mark the box labelled "log in again automatically" or similar.
Ankit Garg
Sheriff

Joined: Aug 03, 2008
Posts: 9304
    
  17

Well I don't think anyone has pointed this out yet. You've used this kind of import statement



in VendingMachine.java. You should have used



like you've done in Snack.java. This makes the code easier to understand as it is easy to figure out from which package is the class coming...


SCJP 6 | SCWCD 5 | Javaranch SCJP FAQ | SCWCD Links
Mick Jones
Greenhorn

Joined: May 21, 2009
Posts: 7
Ok I took comments onboard (I think!) and re-jigged the code, took some methods out of Snack.java + added some new code to get user input etc. I also put files into packages!

I am getting an error at Line 55 of VendingMachine.java and can't work out why it doesn't work! It was working fine before I moved buySnack() out of Snack.java and into VendingMachine.java but can't see why. Just need some fresh eyes.

Can someone help me?

[edit] Was getting confusing so removed this code and new code is below [/edit]
Fred Hamilton
Ranch Hand

Joined: May 13, 2009
Posts: 679
I'm getting a little confused by your use of the word "snack" to mean different things, but anyways, can you justify your use of the term "this" within the buySnack method? I sense maybe a problem there.

edit: this problem I sense wouldn't be the cause of your compile error at line 55 though) That is a case where the first argument of the method call to buySnack, does not match the data type of the first parameter of the actual method declaration.
Mick Jones
Greenhorn

Joined: May 21, 2009
Posts: 7
Ok, the whole Snack thing was starting to confuse a few people.. and me too . So I have now refactored the code so we now have a VendingMachine that has Products! Please see revised code above, I still have the error.

VendingMachine.java


Product.java

Fred Hamilton
Ranch Hand

Joined: May 13, 2009
Posts: 679
the error means the argument in the method call does not match the parameter in the method declaration.

in other words, productMap.get(selection) is a different data type than Map<String, Product> product
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: Rate my code