Page 1 of 2 12 LastLast
Results 1 to 16 of 20

Thread: Constructive criticism required

  1. #1
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts

    Constructive criticism required

    On my program.

    Now, before you have a look at it please understand this. The program wasn't half finished, i know there are many bugs in there and yes there are many silly mistakes in there. It's a very unfinished product. This was my project which has now been handed in its current state.

    Click here

    There's the link. I'd be happy to get some decent feedback please, don't pick at small things that are obvious such as no removal option in the UI, like i said, a lot of features weren't implemented which should have been done

    EDIT: Bear in mind, before i started this project i had no previous experience in any OO whatsoever
    Last edited by Kezzer; 19-05-2005 at 02:03 PM.

  2. #2
    Registered+
    Join Date
    Mar 2005
    Posts
    33
    Thanks
    0
    Thanked
    0 times in 0 posts
    What does it do?

  3. #3
    Senior Member ajbrun's Avatar
    Join Date
    Apr 2004
    Location
    York, England
    Posts
    4,840
    Thanks
    4
    Thanked
    25 times in 13 posts
    I just get a picture with boxes in it, and beneath that, links to some code. Are we supposed to save it for it to work or something?

  4. #4
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    it's a diagramatical explanation of the program.

    The program quotes a person on a box. They give details and it'll return one of 6 box types dependant on the combination of input given or none at all if they don't match a box type. That's why there are 6 box types there.

    The invoice is the invoice, the customer is the customer. All the classes are pretty much self explanatory

  5. #5
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    Quote Originally Posted by ajbrun
    I just get a picture with boxes in it, and beneath that, links to some code. Are we supposed to save it for it to work or something?
    No, it's just to look at

  6. #6
    Agent of the System ikonia's Avatar
    Join Date
    May 2004
    Location
    South West UK (Bath)
    Posts
    3,736
    Thanks
    39
    Thanked
    75 times in 56 posts
    you actually appear to have grasped the idea of OO quite nicley in this example.

    my java's pretty weak but I can see what your doing compared to my C++.

    some of the stuff I don't get why you've done it but you've commented it well and its quite straight forward to follow.

    for your first attempt I'd give it nice credit.
    It is Inevitable.....


  7. #7
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    Well i was hoping from just the source that people would be able to understand it. Like i said, it's nowhere near finished and should be easy to understand. The OO concept isn't perfect as the names of the sub-types of Box are incorrect

  8. #8
    Sam
    Sam is offline
    Registered+
    Join Date
    Oct 2003
    Posts
    22
    Thanks
    0
    Thanked
    0 times in 0 posts
    Only a quick look, but you shouldn't have different boxes as explicit classes. That's why you create instances of a class. Have a box class, and then define the boxes that exist as instances of that class. That's a better way to think about OOP.

  9. #9
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    No Sam, not in this case, the behaviour is different between the classes and the whole reason for this project was to use inheritance. I could have done it differently and just created instances of the one class but the actual specification DOES require inheritance

  10. #10
    Sam
    Sam is offline
    Registered+
    Join Date
    Oct 2003
    Posts
    22
    Thanks
    0
    Thanked
    0 times in 0 posts
    I didn't really take too long a look, is it just calculating cost differently? I notice some of them have identical code except for a constant and that's poor.

    Also, lose the DetermineBox class and put it in a static method inside Box.

  11. #11
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    The reason why i kept DetermineBox seperate was because i was meant to be implementing a strategy design pattern but never got round to it. I will be completely refining all the code in this program when i have some more time.

    None of them have any constants in them. In a way yes, it's calculating the cost differently but like i said, i could have implemented this without inheritance happily, but logically it should be done using inheritance it's just my implementation is poor. There's a reason for everything in this program and i know it looks like poor inheritance but this project was set out by a professor who has written like 5 books on programming. There's difference in behaviour and there should be different in state but i didn't see the point

  12. #12
    Taz
    Taz is offline
    Senior Member Taz's Avatar
    Join Date
    Jan 2005
    Location
    London
    Posts
    2,152
    Thanks
    57
    Thanked
    29 times in 27 posts
    • Taz's system
      • Motherboard:
      • Gigabyte Z270 HD3P
      • CPU:
      • Intel Core i5 7600K
      • Memory:
      • Corsair CMK16GX4M2B3200C16R Vengeance LPX 16 GB
      • Storage:
      • Samsung 960 EVO M.2-2280 500GB (PCIe) + 1TB Sandisk Ultra II SSD (SATA)
      • Graphics card(s):
      • Asus NVIDIA GeForce GTX 1070 OC
      • PSU:
      • Corsair CS550M 550W Hybrid
      • Case:
      • NZXT Source 340
      • Operating System:
      • Windows 10 Pro
      • Monitor(s):
      • 34" Asus Designo Curve MX34VQ UWQHD Monitor
      • Internet:
      • Virgin Media M350
    Kezzer, i'm new to Java programming so i've had a look at your code more in the hope of learning from it!

    I was wondering if you could replace the following code in box.java:

    /** returns the cost of card per square metres dependant on gradeOfCard */
    68 public double getSquareMeterCost() {
    69 if(gradeOfCard == 1)
    70 return 0.08;
    71 else if (gradeOfCard == 2)
    72 return 0.10;
    73 else if (gradeOfCard == 3)
    74 return 0.14;
    75 else if (gradeOfCard == 4)
    76 return 0.18;
    77 else if (gradeOfCard == 5)
    78 return 0.24;
    79 else
    80 return 0.0;
    81 }

    with a 'switch' statement?

    Also in DetermineBox.java, could the following code be made more elegant:

    public Box determineBoxType() {
    29 if(gradeOfCard >= 1 && gradeOfCard <= 3 && print == 0
    30 && bottom == false && corners == false) {
    31 return new Box(gradeOfCard, sealTop, width, height, length);
    32 }
    33 else if(gradeOfCard >= 1 && gradeOfCard <= 4 && print == 1
    34 && bottom == false && corners == false) {
    35 return new BoxTwo(gradeOfCard, sealTop, width, height, length);
    36 }
    37 else if(gradeOfCard >= 2 && gradeOfCard <= 4 && print == 2
    38 && bottom == false && corners == false) {
    39 return new BoxThree(gradeOfCard, sealTop, width, height, length);
    40 }
    41 else if(gradeOfCard >= 2 && gradeOfCard <= 5 && print == 3
    42 && bottom == false && corners == false) {
    43 return new BoxFour(gradeOfCard, sealTop, width, height, length);
    44 }
    45 else if(gradeOfCard >= 2 && gradeOfCard <= 5 && print == 3
    46 && bottom == true && corners == false) {
    47 return new BoxFive(gradeOfCard, sealTop, width, height, length);
    48 }
    49 else if(gradeOfCard >= 3 && gradeOfCard <= 5 && print == 3
    50 && bottom == true && corners == true) {
    51 return new BoxSix(gradeOfCard, sealTop, width, height, length);
    52 }
    53 else
    54 return null;
    55 }

    I'm not sure how you'd do it, just a thought really. Other than that i'll definetely be learning more about Java from your program.

  13. #13
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    Hehe, i know exactly why it's good to have a switch statement but i'd prefer for you to tell me if it seems you're learning

  14. #14
    Large Member
    Join Date
    Apr 2004
    Posts
    3,720
    Thanks
    47
    Thanked
    99 times in 64 posts
    It seems a little odd to extend each different Box. It defeats the point of the whole OO idea. A box is a box, you should instantiate a box type from the Box class, passing it it's parameters. I'm sure you could have implemented inheritance in a more worthwile manner, extending JFrame for your GUI for example.
    To err is human. To really foul things up ... you need a computer.

  15. #15
    Senior Member Kezzer's Avatar
    Join Date
    Sep 2003
    Posts
    4,863
    Thanks
    12
    Thanked
    5 times in 5 posts
    No yamangman, there are literally 6 different types of box in the real world. I'm telling you now, it's 100% definitely correct

  16. #16
    Large Member
    Join Date
    Apr 2004
    Posts
    3,720
    Thanks
    47
    Thanked
    99 times in 64 posts
    I don't get you. You ask for constructive criticism, I point out a poor programming practice, and then you appear to take it personally.
    To err is human. To really foul things up ... you need a computer.

Page 1 of 2 12 LastLast

Thread Information

Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)

Similar Threads

  1. Whats required?
    By Crispy in forum PC Hardware and Components
    Replies: 3
    Last Post: 19-11-2004, 07:32 PM
  2. Urgent help required chaps...
    By Knoxville in forum PC Hardware and Components
    Replies: 7
    Last Post: 09-11-2004, 09:37 PM
  3. Urgent help required chaps...
    By Knoxville in forum Help! Quick Relief From Tech Headaches
    Replies: 3
    Last Post: 09-11-2004, 04:57 PM
  4. Dabs.com - INFO REQUIRED {Dunno where this should go}
    By MuTTy_Hc in forum Retail Therapy and Bargains
    Replies: 14
    Last Post: 03-03-2004, 12:27 AM

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •