14 Replies - 1007 Views - Last Post: 08 February 2013 - 02:56 PM Rate Topic: ***** 1 Votes

#1 lepazoga  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 9
  • Joined: 06-February 13

Finished programming an assignment, what could I do better?

Posted 06 February 2013 - 10:57 PM

I am posting this because I always feel like I Could do something better with my code. I feel like I'm too elementary, do you have any suggestions about what I can do better? (This is my first time posting, so I hope this is a suitable question and wont get flagged.)

Main Method Side
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace InternetMerchandise
{
    class Program
    {
        static void Main(string[] args)
        {
        // Declaring Variables and Methods
            double totalPurchases;
            totalPurchases = GetTotalPurchases();
            ShippingCost MyCost = new ShippingCost(totalPurchases);

        // Results for showing shipping charges

            MyCost.SetCharges(totalPurchases);
            Console.ReadLine();
            Console.WriteLine("The Shipping Charges is: {0:C}", MyCost.ShippingCharges);
            Console.ReadLine();   
        }

     // method to obtain Total Purchases
        public static double GetTotalPurchases()
        {
            double totalPurchases;
            Console.WriteLine(" Enter the Price of goods purchased\n ");
            totalPurchases = double.Parse(Console.ReadLine());
            return totalPurchases;
        }
   }
}






using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace InternetMerchandise
{
    class ShippingCost
   
        {
            private double shippingCharges;
            private double totalPurchases;

        // Default Constructor
            public ShippingCost()
            {
            }

        // Constructor with One argument
            public ShippingCost(double price)
            {
                totalPurchases = price;
            }

        // Getters and Setters
            public double TotalPurchases
            {
                set { totalPurchases = value; }
                get { return totalPurchases; }
            }

            public double ShippingCharges
            {
                set { shippingCharges = value; }
                get { return shippingCharges; }
            }

        // Determine how much customer is to be charged per purchase price
            public void SetCharges(double totalPurchases)
            {

                if ((totalPurchases > 0) && (totalPurchases <= 250))
                    shippingCharges = 5;
                else if
                    ((totalPurchases > 250) && (totalPurchases <= 500))
                    shippingCharges = 8;

                else if ((totalPurchases > 500) && (totalPurchases <= 1000))
                    shippingCharges = 10;
                else if
                    ((totalPurchases>1000) && (totalPurchases <= 5000))
                    shippingCharges = 15;

                else if ((totalPurchases > 5000))
                    shippingCharges = 20;

            }
        }
    }



Is This A Good Question/Topic? 0
  • +

Replies To: Finished programming an assignment, what could I do better?

#2 raghav.naganathan  Icon User is offline

  • Perfectly Squared ;)
  • member icon

Reputation: 408
  • View blog
  • Posts: 1,440
  • Joined: 14-September 12

Re: Finished programming an assignment, what could I do better?

Posted 06 February 2013 - 11:55 PM

Well, your getter and setter methods don't seem to be used at all in your program...I would suggest you remove them completely from your program.

Well, if you want them to remain, you need to note that the methods need to have () at the end

 public double TotalPurchases()
27	            {
28	                set { totalPurchases = value; }
29	                get { return totalPurchases; }
30	            }



regards,
Raghav

This post has been edited by raghav.naganathan: 06 February 2013 - 11:56 PM

Was This Post Helpful? 0
  • +
  • -

#3 ThrowsException  Icon User is offline

  • D.I.C Head

Reputation: 33
  • View blog
  • Posts: 83
  • Joined: 21-February 12

Re: Finished programming an assignment, what could I do better?

Posted 06 February 2013 - 11:59 PM

View Postraghav.naganathan, on 07 February 2013 - 01:55 AM, said:

Well, your getter and setter methods don't seem to be used at all in your program.

Well, if you want them to remain, you need to note that the methods need to have () at the end

This is incorrect these are not getters and setters, these are properties and one is being used in line 21 of his main method.

Your program is pretty simple and straight forward. Is there anything in particular you feel like your code is lacking or you don't feel like you're progressing enough?
Was This Post Helpful? 2
  • +
  • -

#4 ThrowsException  Icon User is offline

  • D.I.C Head

Reputation: 33
  • View blog
  • Posts: 83
  • Joined: 21-February 12

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:08 AM

One thing you can do to shorten your code a little is to leverage your properties you have. Right now you are you using public properties with private backers

private double shippingCharges;
public double ShippingCharges
{
   set { shippingCharges = value; }
   get { return shippingCharges; }
}



when instead you can just do this
public double ShippingCharges { get; set; }


This change is almost completely stylistic though and does not effect the flow of your code. Some programmers prefer the more verbose method with the private backing fields while others prefer the more shorthand notation. Really matters what you are trying to accomplish in the code as well.
Was This Post Helpful? 2
  • +
  • -

#5 raghav.naganathan  Icon User is offline

  • Perfectly Squared ;)
  • member icon

Reputation: 408
  • View blog
  • Posts: 1,440
  • Joined: 14-September 12

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:13 AM

Well, what I meant was to only remove the lines 26 to 30...I never mentioned anything at all about the ShippingCharges...

regards,
Raghav
Was This Post Helpful? 1
  • +
  • -

#6 lepazoga  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 9
  • Joined: 06-February 13

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:22 AM

Thanks for the input. I really appreciate it. I guess I'm looking to make things shorter. I look at my friend's code, he's a CS major, and everything just seems so elegant, concise, etc. I feel like I Just have so much extra code that isn't needed.
Was This Post Helpful? 0
  • +
  • -

#7 ThrowsException  Icon User is offline

  • D.I.C Head

Reputation: 33
  • View blog
  • Posts: 83
  • Joined: 21-February 12

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:25 AM

I don't know what your skill or comfort level is but don't harp to much on writing super elegant or pretty code right away. As you write more and more code you'll find yourself writing cleaner, more concise code. its a process of continual improvement but you're already taking the first step in reviewing your code and asking how can I make it better.
Was This Post Helpful? 2
  • +
  • -

#8 Momerath  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 1010
  • View blog
  • Posts: 2,444
  • Joined: 04-October 09

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 04:14 AM

You do this:
double totalPurchases;
totalPurchases = GetTotalPurchases();
ShippingCost MyCost = new ShippingCost(totalPurchases);


Pick a way to do this (declare on one line, set on another; declare and set on one line) and stick with it. Different programs may use different ways.


totalPurchases = double.Parse(Console.ReadLine());


What happens when the user types in 'tuesday'? Use TryParse over Parse unless you know that the input cannot be invalid (or if it is you want to raise an exception).


Your code
if ((totalPurchases > 0) && (totalPurchases <= 250))
    shippingCharges = 5;
else if ((totalPurchases > 250) && (totalPurchases <= 500))
    shippingCharges = 8;
else if ((totalPurchases > 500) && (totalPurchases <= 1000))
    shippingCharges = 10;
else if ((totalPurchases>1000) && (totalPurchases <= 5000))
    shippingCharges = 15;
else if ((totalPurchases > 5000))
    shippingCharges = 20;

does too many checks. In the first 'else if' you already know that it is greater than 250, so why are you checking if it is?

if (totalPurchases <= 250) shippingCharges = 5;
else if (totalPurchases <= 500) shippingCharges = 8;
else if (totalPurchases <= 1000) shippingCharges = 10;
else if (totalPurchases <= 5000) shippingCharges = 15;
else shippingCharges = 20;


GetTotalPurchases() should already have validated that the numbers is valid (not less than or equal to zero).

Lines 20 and 22 in your code. You shouldn't have Console.ReadLine() statements without some form of prompt to the user. How are the supposed to know that they should hit enter to make the code continue? For all they know you are doing some long running calculation.

You shouldn't have to call the method SetCharges. The amount is part of the object already, so do the calculation when the user requests the shipping charges
public double ShippingCharges {
    get { return SetCharges(this.totalPurchases); }
}

This also requires that you make SetCharges return a value rather than having it set the property. I also wouldn't name it SetCharges (you aren't setting anything, you are getting them) and I wouldn't have it require a parameter (it has access to the objects private variables already).

There shouldn't be a 'set' in ShippingCharges as it's based off the objects state.


Edit: typos

This post has been edited by Momerath: 07 February 2013 - 04:17 AM

Was This Post Helpful? 4
  • +
  • -

#9 raghav.naganathan  Icon User is offline

  • Perfectly Squared ;)
  • member icon

Reputation: 408
  • View blog
  • Posts: 1,440
  • Joined: 14-September 12

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 04:20 AM

Excellent answer Momerath

Nevertheless, I would like to disagree with you on one small thing though.

if (totalPurchases <= 250) shippingCharges = 5; //user can type in 0 or negative values and still get shippingCharges as 5


So, to avoid that,
if ((totalPurchases > 0) && (totalPurchases <= 250))

needs to be the first condition.

regards,
Raghav

This post has been edited by raghav.naganathan: 07 February 2013 - 04:20 AM

Was This Post Helpful? 1
  • +
  • -

#10 tlhIn`toq  Icon User is offline

  • Please show what you have already tried when asking a question.
  • member icon

Reputation: 5436
  • View blog
  • Posts: 11,656
  • Joined: 02-June 10

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 08:19 AM

View PostThrowsException, on 07 February 2013 - 01:08 AM, said:

One thing you can do to shorten your code a little is to leverage your properties you have. Right now you are you using public properties with private backers

private double shippingCharges;
public double ShippingCharges
{
   set { shippingCharges = value; }
   get { return shippingCharges; }
}



when instead you can just do this
public double ShippingCharges { get; set; }


This change is almost completely stylistic though and does not effect the flow of your code. Some programmers prefer the more verbose method with the private backing fields while others prefer the more shorthand notation. Really matters what you are trying to accomplish in the code as well.


While this is true for this simple example, I personally see nothing wrong with using the more complex form of the property. Very soon you will start using the properties for more complex things. As soon as you actually *do* something in the set method you have to have a backing variable. So why train to do it one way then have to re-train your brain.

My typical property looks look this. I use a snippet to handle almost all of the insertion. So all I have to type is nuprop{tab}, enter the type, enter the name and everything else is done for me.

Posted Image

Posted Image

Posted Image

I find that I do this for all properties, all the time even if it is overkill for the need at the time. More often than not, I have a need for the property to become more complex.

Also, in the case of WPF projects you have to have the property raise the PropertyChanged event to comply with the INotifyPropertyChanged interface so you can bind the property to a WPF Window control.
Was This Post Helpful? 3
  • +
  • -

#11 Momerath  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 1010
  • View blog
  • Posts: 2,444
  • Joined: 04-October 09

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:32 PM

View Postraghav.naganathan, on 07 February 2013 - 03:20 AM, said:

Nevertheless, I would like to disagree with you on one small thing though.

if (totalPurchases <= 250) shippingCharges = 5; //user can type in 0 or negative values and still get shippingCharges as 5


So, to avoid that,
if ((totalPurchases > 0) && (totalPurchases <= 250))

needs to be the first condition.


I covered that in the comment after that block. IMO, validation should occur when the value is input, not sometime later in the code. If the user typed in -1000, for example, the input code should re-ask for a value after telling them they can't buy -1000 worth of items.
Was This Post Helpful? 2
  • +
  • -

#12 tlhIn`toq  Icon User is offline

  • Please show what you have already tried when asking a question.
  • member icon

Reputation: 5436
  • View blog
  • Posts: 11,656
  • Joined: 02-June 10

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:52 PM

This is where I like (for WinForms) the ErrorProvider.

Posted Image


It provides visual indication that you need to provide correct values, doesn't hold up the process of inputting, one control can update when another control's value changes, provides ToolTip explanation rather than a big honking messagebox.

Its not something I would expect in a student's first project as we have here, but its a lot more elegant than bullying the user after each value is input and puts the validation in a single place for better maintenance.

This post has been edited by tlhIn`toq: 07 February 2013 - 12:54 PM

Was This Post Helpful? 1
  • +
  • -

#13 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2241
  • View blog
  • Posts: 9,412
  • Joined: 29-May 08

Re: Finished programming an assignment, what could I do better?

Posted 07 February 2013 - 12:53 PM

There should be validation checks in the data layer, if the data is invalid then raise an exception, or raise an invalid data event.

The repeating bounds check, suggests to me I could write a function (or more likely an extension method) to handle it for me.

If value.IsBetween(0, 250) Then 
...



Also the working out of the shipping charges, could be separated out into it on function.

Function CalculateCharges(Value As Double) As Double
  If value < 0 Then Throw New MyException
  If value.IsBetween(0,250) Then Return 5
  If value.IsBetween(250,500) Then Return 10
  ...
  Return 100
End Function

Or even better is to abstract that away into an interface implementation.
Interface IShippingChargesCalc
  Function CalculateCharges(Value As Double) As Double
End Interface


This post has been edited by AdamSpeight2008: 07 February 2013 - 01:12 PM

Was This Post Helpful? 1
  • +
  • -

#14 MathewS  Icon User is offline

  • D.I.C Regular

Reputation: 18
  • View blog
  • Posts: 342
  • Joined: 14-May 02

Re: Finished programming an assignment, what could I do better?

Posted 08 February 2013 - 09:12 AM

View Postraghav.naganathan, on 07 February 2013 - 11:20 AM, said:

Excellent answer Momerath

Nevertheless, I would like to disagree with you on one small thing though.

if (totalPurchases <= 250) shippingCharges = 5; //user can type in 0 or negative values and still get shippingCharges as 5


So, to avoid that,
if ((totalPurchases > 0) && (totalPurchases <= 250))

needs to be the first condition.

regards,
Raghav


You would need to add that to each if statement, otherwise if someone put in -1 the first would fail but the second would pass.

Could just as easily do
if (totalPurchases <= 0)
{
  shippingCharges = 0;
  return;
}

if (totalPurchases <= 250) 
  shippingCharges = 5;


Was This Post Helpful? 0
  • +
  • -

#15 lepazoga  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 9
  • Joined: 06-February 13

Re: Finished programming an assignment, what could I do better?

Posted 08 February 2013 - 02:56 PM

There are so many posts lol. I had no idea I'd get these kind of responses. I went with what Momereth recommended that I should do with my if-then statement. Anyway, thanks everyone for your input. Hopefully I'll understand soon everything you are talking about ;o
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1