constructor question

  • Thread starter Thread starter Mike P
  • Start date Start date
M

Mike P

I am trying to create a class where I will validate each parameter sent
to the contructor. I need to know which of the parameters (if any) were
invalid, so do I need to throw a user defined exception at that point so
that I know exactly which parameter had an invalid value?

I am new to creating class objects so I'm unsure about where I should do
my validation and how to trap errors. Here is my code :

private string strCardType;
private string strCardNumber;


public creditcard(string strCardType, string strCardNumber, etc...)
{
try
{
this.CardType = strCardType;
}
catch
{

}

this.CardNumber = strCardNumber;
}


public string CardType
{
get
{
return this.strCardType;
}
set
{
if ((this.strCardType.ToUpper() != "VISA") &&
(this.strCardType.ToUpper() != "MASTERCARD")
&& (this.strCardType.ToUpper() != "SWITCH") &&
(this.strCardType.ToUpper() != "DELTA")
&& (this.strCardType.ToUpper() != "SOLO") &&
(this.strCardType.ToUpper() != "JCB")
&& (this.strCardType.ToUpper() != "VISA ELECTRON") &&
(this.strCardType.ToUpper() != "MAESTRO"))
{
this.strCardType = value;
}
else
{
throw new Exception();
}
}
}

public string CardNumber
{
get
{
return this.strCardNumber;
}
set
{
this.strCardNumber = value;
}
}


Any assistance would be really appreciated.


Cheers,

Mike
 
Mike P said:
I am trying to create a class where I will validate each parameter sent
to the contructor. I need to know which of the parameters (if any) were
invalid, so do I need to throw a user defined exception at that point so
that I know exactly which parameter had an invalid value?
Click to expand...

The best thing to throw would be ArgumentException, which allows you to
specify which argument is invalid.

By the way - in your CardType setter, you're testing whether the
*existing* value is valid or not, rather than the value you're about to
set. Is that what you intended? It would also be a good idea to call
ToUpper *once*, and either use a Hashtable of valid values, or perhaps
a switch statement.
 
John,

I'm not sure where I should be testing the value coming in, in the
contructor, the get or the set. I would've thought that if the value
sent through is invalid then I should be trapping it in the set, so that
I catch it in the constructor?


Regards,

Mike
 
Mike P said:
I'm not sure where I should be testing the value coming in, in the
contructor, the get or the set. I would've thought that if the value
sent through is invalid then I should be trapping it in the set, so that
I catch it in the constructor?
Click to expand...

You shouldn't be catching it at all. You should be testing it in the
property, and throwing an exception there.
 
Jon Skeet wrote:

[...snip...]
You shouldn't be catching it at all. You should be testing it in the
property, and throwing an exception there.
Click to expand...
[...snip...]

Well, I'm sure you meant the OP should catch the exception _somewhere_ ;-)
Catching it inside the constructor wouldn't be appropriate. I'd catch the
exception wherever I tried to instanciate the CreditCard instance:

try
{
CreditCard something = new CreditCard("MASTER", "1234123412341234"...);
}
catch (ArgumentException e)
{
MessageBox.Show(this, "Credit card data invalid: " + e.Message);
};
 
Thanks Mike. Yes, this is what I am doing now :

public class CreditCard
{
private string strCardType;
private string strCardNumber;
private string strExpiryMonth;

//#region constructors
public CreditCard(string strCardType, string strCardNumber, string
strExpiryMonth,
string strExpiryYear)
{
this.CardType = strCardType;
this.CardNumber = strCardNumber;
this.ExpiryMonth = strExpiryMonth;

}
//#endregion

//#region public properties
public string CardType
{
get
{
return this.strCardType;
}
set
{
if ((value.ToUpper() != "VISA") && (value.ToUpper() != "MASTERCARD")
&& (value.ToUpper() != "SWITCH") && (value.ToUpper() != "DELTA")
&& (value.ToUpper() != "SOLO") && (value.ToUpper() != "JCB")
&& (value.ToUpper() != "VISA ELECTRON") && (value.ToUpper() !=
"MAESTRO"))
{
throw new CardTypeException();
}
else
{
this.strCardType = value;
}
}
}

public string CardNumber
{
get
{
return this.strCardNumber;
}
set
{
Match mthCardNumber;
Regex objCardNumber = new
Regex("^[0-9]?[0-9]?[0-9]?[0-9]?[0-9]?[0-9]?[0-9]?[0-9]{12}$");

mthCardNumber = objCardNumber.Match(value);

if (mthCardNumber.Success == false)
{
throw new CardNumberException();
}
else
{
this.strCardNumber = value;
}
}
}

public string ExpiryMonth
{
get
{
return this.strExpiryMonth;
}
set
{
if ((value != "01") && (value != "02")
&& (value != "03") && (value != "04")
&& (value != "05") && (value != "06")
&& (value != "07") && (value != "08")
&& (value != "09") && (value != "10")
&& (value != "11") && (value != "12"))
{

}
else
{

}
}
}
//#endregion




}

And I am instantiating the class here :

try
{
CreditCard ccPackage = new CreditCard(strCardTypePackage,
strCardNoPackage, strExpiryMonthPackage, strExpiryYearPackage);
}
catch (CardTypeException)
{
intErrorNumber = 211;
strErrorDescription = Get_Error_Description(intErrorNumber);
Create_XML_Error_Package(bytMessageID, strCUG, intErrorNumber,
strErrorDescription);
Print_XML_Result_To_Screen();

return;
}
catch (CardNumberException)
{
intErrorNumber = 212;
strErrorDescription = Get_Error_Description(intErrorNumber);
Create_XML_Error_Package(bytMessageID, strCUG, intErrorNumber,
strErrorDescription);
Print_XML_Result_To_Screen();

return;
}

One thing I still don't get is why would I want to use a hashtable
rather than my if statement :

if ((value.ToUpper() != "VISA") && (value.ToUpper() != "MASTERCARD")
&& (value.ToUpper() != "SWITCH") && (value.ToUpper() != "DELTA")
&& (value.ToUpper() != "SOLO") && (value.ToUpper() != "JCB")
&& (value.ToUpper() != "VISA ELECTRON") && (value.ToUpper() !=
"MAESTRO"))
{
}

Maybe somebody could enlighten me on this, and show me an example.


Cheers,

Mike
 
Michael Voss said:
[...snip...]
You shouldn't be catching it at all. You should be testing it in the
property, and throwing an exception there.
Click to expand...
[...snip...]

Well, I'm sure you meant the OP should catch the exception _somewhere_ ;-)
Click to expand...

Not necessarily, and certainly not necessarily specifically.
Catching it inside the constructor wouldn't be appropriate. I'd catch the
exception wherever I tried to instanciate the CreditCard instance:

try
{
CreditCard something = new CreditCard("MASTER", "1234123412341234"...);
}
catch (ArgumentException e)
{
MessageBox.Show(this, "Credit card data invalid: " + e.Message);
};
Click to expand...

I wouldn't. I'd put an exception handler far further up, almost
certainly.
 
Mike P said:
One thing I still don't get is why would I want to use a hashtable
rather than my if statement :

if ((value.ToUpper() != "VISA")
&& (value.ToUpper() != "MASTERCARD")
&& (value.ToUpper() != "SWITCH") && (value.ToUpper() != "DELTA")
&& (value.ToUpper() != "SOLO") && (value.ToUpper() != "JCB")
&& (value.ToUpper() != "VISA ELECTRON") && (value.ToUpper() !=
"MAESTRO"))
{
}

Maybe somebody could enlighten me on this, and show me an example.
Click to expand...

It's easy enough to do:

class Foo
{
static readonly Hashtable ValidCardTypes = new Hashtable();

static Foo()
{
string[] types = {"Mastercard", "Switch", "Solo"}; // etc
foreach (string x in types)
{
string type = x.ToUpper();
ValidCardTypes[type]=type;
}
}

string strCardType;
public string CardType
{
get
{
return this.strCardType;
}
set
{
if (!ValidCardTypes.ContainsKey(value.ToUpper()))
{
throw new CardTypeException();
}

strCardType = value;
}
}
}

Advantages:
1) If you have a lot of card types in the future, a hashtable look-up
can end up being faster than testing each value
2) You can very easily go from the code above to loading the initial
list from a resource or whatever
3) You're only upper-casing the value once :)

Using just a string array would be fine too, although it would lose
advantage 1. The difference probably wouldn't be significant until you
had many, many card types though.
 
Thanks for the example Jon, I'll definitely look into using hashtables
further!


Cheers,

Mike
 
I have one more question...my finished class is going to have 5 optional
parameters, which will mean 31 different overloads of this class
(optional credit card details like issue number etc). Surely there must
be an easier way of instantiating my class than having a massive switch
statement with all the different possible overloads??


Thanks,

Mike
 
Mike P said:
I have one more question...my finished class is going to have 5 optional
parameters, which will mean 31 different overloads of this class
(optional credit card details like issue number etc). Surely there must
be an easier way of instantiating my class than having a massive switch
statement with all the different possible overloads??
Click to expand...

Well, you don't need *all* the permutations - just commonly used ones,
documenting what you should pass instead of the optional parameter if
you don't want to really specify it.
 
Back
Top