Results 1 to 2 of 2
  1. #1
    Join Date
    Feb 2015
    Posts
    1

    C# 101 - Vending Machine Homework CHECK

    Hey guys, I completed the first lecture for C#101 Spring 2012, and hopefully did the Vending Machine program correctly and would like some feedback. It works for the most part but I would like your advice on what I need to improve. The only problem is that I think I could have used a loop if the incorrect option was chosen, but since we haven't gone over loops yet, I didn't want to jump into it.

    Here is the .cs file : http://s000.tinyupload.com/?file_id=...79223855845033
    Screenshot :


    Any help is greatly appreciated.
    Thanks, Yuriy.

  2. #2
    Join Date
    May 2011
    Posts
    95
    Hello!! I suppose I can give a bit of feedback, even though this was posted a long time ago. I know that you're just learning so I'll leave out any design feedback. Some of the feedback will also be purely style related.

    - Excess calls to Console.WriteLine()

    Avoid having so many Console.WriteLine() calls simply for formatting. Doing this has several effects. First, it bloats your code. Secondly, its not commonly done that way, so its confusing to those reading your code as to why there are so many blank Console.WriteLine calls. Third, none of those calls are free, so you're invoking extra functions that you don't need to invoke. An analogy would be, you wanted to have a dog, so you bought a whole pet store. You don't need to buy the pet store just to own a dog.

    Instead, you should use "\n" to put a new line where you want them to go. So you would have:
    Code:
    Console.WriteLine("You have $" + money + " to spend!\n\n");
    Each "\n" will give you a new line. You also don't need to call Console.WriteLine if you don't want to, there is also a Console.Write, which will write on the current line without making a new one. This might give you more control and allow you to make new lines simply by using "\n", and not incurring extra function calls. You should always try and simplify and get the job done with as little function calls as possible, while still being readable and efficient.

    Use Else
    Your last if statement on line 53, should be an else statement. In your logic, if the user presses anything other than 1, 2, 3, or 4, they have pressed an incorrect value! It should be reflected this way in your logic, as it makes it easier to read and quickly understand the code.

    Code:
    // Option 1
    if (choice > 4 || choice < 1)
       // Do a thing
    
    // Option 2
    else
       // Do a thing
    Option 1 is saying to the reader, "There is a condition that we need to explicitly consider in this statement. If that condition is met, then it is an error". What you actually mean is, "If none of the handled conditions were met, then its an error", so you should use Option 2.

    However!!! Some programmers like to handle the error case first, and return or break out of the logic if the error was met. This way, we can quickly get bogus input out of the way and then we can read the "happy path" code without having to worry about error cases. Consider this refactoring:

    Code:
    while (choice > 4 || choice < 1)
       // Report an error
       // Here we can restart the loop until we get a valid input from the user
    
    if (choice == 1)
       // Do a thing
    
    else if (choice == 2)
       // Do a thing
    
    else if (choice == 3)
       // Do a thing
    
    else if (choise == 4)
       // Do a thing
    I'm not sure if you have seen while yet, but that will repeatedly perform the logic inside that block until the condition presented fails. This means that as long as the user inputs wrong values, it will continue to execute that block. It also means that inside that block, you should make sure to get a new value from the user inside that while loop!

    You might notice the use of else in the code snippet above. You should definitely make those statements else-if statements!! What you are doing by leaving them as if statements is ensuring that each statement is being checked no matter what. So currently, here is what your program does:

    Code:
    // choice = 2
    // Is choice == 1? No? Move on.
    // Is choice == 2? Yes! Cool, do my things for 2.
    // Is choice == 3? No? Move on.
    // Is choice == 4? No? Move on.
    // Is choice > 4 || < 1? No? Move on.
    Hopefully you can see that we could have stopped checking once we knew it was 2. Your logic indicates to the reader that, even though we successfully executed code at choice == 2, there is a possibility of it being something else since we're still explicitly checking even after we already know that it can never be anything other than 2 at that point. This is not what you want. What you want is for your program to do this:

    Code:
    // Is choice == 1? No? Move on.
    // Is choice == 2? Yes? Cool, do my things for 2.
    // Program exit. (or loop repeat, however you want to handle the end of that process).
    You do this with else - if. It will only check subsequent else -if statements if previous ones have have failed. This makes your intention more clear, and cuts down the amount of checks your program needs to do.


    Keep in mind that these items of feedback literally do not matter for the performance of the program you wrote. However, while you are learning you should learn best practices and keep these things in mind and use them even though it may not matter for trivial programs. When you get to where you're working in large code bases with thousands and thousands of source files in a single project, it will matter very much!! There is value for sure in learning to think about these things early on!

    Hopefully this was useful to you Good job and keep up the good work!!
    Last edited by frostbytes89; 03-29-2015 at 02:37 PM.

Posting Permissions

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