I need help improving my code
-
It's obvious the code shown here depends on a context which you don't show; a guess would be that this code is being used with a 3rd. party Control: 1. there's no override named 'Validate for either a WinForm, or a sub-classed TextBox. 2. you never do anything with the Control passed as a parameter. 3. the string assignments to 'ErrorText use (probably) a Property in the context here; a guess would be that the 'setter of that Property has "side-effects." 4. the 'Value parameter here comes in as an Object, but clearly it is a string. If I thought the user entering text here was going to type in an integer most of the time:
Int32 _minValue = 0;
Int32 _maxValue = 100;
Int32 trialvalue;private bool checkInt(object val)
{
string strval = val.ToString();if (Int32.TryParse(strval, out trialvalue)) { // you have an integer // in range: good to go if (trialvalue >= \_minValue && trialvalue <= \_maxValue) return true; // handle out of range error here // ErrorText = outOfRangeErrorText; // define this somewhere return false; } // analyze for other errors return false;}
Comments: 1. measuring the "complexity" of this code is probably useless since (guess) its structure is dictated by the whatever you over-ride, and depends on that for error-reporting. 2. I'd certainly want to get the string creation used for the error messages here outside of the method; some of them can be declared string constants.
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
Thank you, Yes you are right, it was inherited from a third party control, but the parameter Control will be handled by the base class, and the ErrorText is also a property. basically the type of the parameter Value will defend on how it was implemented. but in this case, while the input will came from a user using either a textbox or other winform controls. so it was a string then will be validated if it is a integer. here is my solution
public override bool Validate(Control control, object value)
{
if (value == null && !_IsAllowNull)
{
ErrorText = "Please provided valid number without a decimal point.";
return false;
}if (value.ToString().Contains(".")) { ErrorText = "Decimal value is not allowed"; return false; } if (!value.IsNumber()) { ErrorText = "Please provided valid number without a decimal point."; return false; } if (value.ToInt() < \_minValue || value.ToInt() > \_maxValue) { ErrorText = "Value should not be greater than " + \_maxValue + " or less than " + \_minValue; return false; } return true;}
the complexity of this solution just drop 3 point. but I like your solution, I will try to implement it. once again, thank you.
-
There are a few things you can do. BillWoodRuff gave a good example. - the control variable is never used inside the function. - Is there a reason to pass value as an object? In addition I would never use a keyword (value) as a variable name. (unless the autofill gave it that name of course) - I never program multiple exit points in a function, often it is harder to debug. Better to set a boolean and return that at the end. (although that's more a matter of taste) - Keep the { }. It will be easier to add a statement if a modification is needed. - int.TryParse is a very good function. I also often use Convert.ToInt32, but make sure you know what it returns in case of null values or invalid integer strings/objects. (read MSDN doc) - In some countries the decimal sign is not a ".", but a komma ",". - You don't need _minValue or _maxValue you have int.MinValue and int.MaxValue. - An object variable has an IsNumber() and ToInt() function? (not in version 4.5) Hope this helps.
V.
(MQOTD rules and previous solutions)hmm about the decimal sign, its new to me that it is a comma to other country. BTW, basically the initial value of _minValue and _maxValue is from the int.MinValue and int.MaxValue, but in some point it could be possible that i will limit the value the user can entered, and it was defined in the constructor.
private int _maxValue = int.MaxValue;
private int _minValue = int.MinValue;
private bool _IsAllowNull;public ValidateInteger(bool isAlloweNull = true) { \_IsAllowNull = isAlloweNull; } public ValidateInteger(int min, int max, bool isAlloweNull = false) { \_maxValue = max; \_minValue = min; \_IsAllowNull = isAlloweNull; }while the IsNumber() and ToInt() is a custom extension. Thank you
-
Personally I would put a mask on the control to only allow the entering of digits 0-9, then all you have to check is that the entry is not outside the max and min values.
========================================================= I'm an optoholic - my glass is always half full of vodka. =========================================================
Yes I can do this. but i can easily miss it. thanks
-
I'd say it's not too bad. Drop all the elses, they are not needed as each condition returns out. That will remove some of the indentation. If value is null and nulls are allowed, then it'll throw a null exception on the ToString(). The error message for the null check is wrong. The IsNumber() and ToInt() functions are interesting, as in they don't exist on object. Extension methods? The assumption seems to be that the number is actually a string representation of a number, in which case first confirm it is a string, then do a int.TryParse to extract it...
Regards, Rob Philpott.
yup the IsNumber() and ToInt() is a custom extension, basically the IsNumber() will identify the string if it can be a number(either a decimal,int and so on), while the ToInt() as you expected will convert a value to int. BTW thank you, for your suggestion
-
Thank you, Yes you are right, it was inherited from a third party control, but the parameter Control will be handled by the base class, and the ErrorText is also a property. basically the type of the parameter Value will defend on how it was implemented. but in this case, while the input will came from a user using either a textbox or other winform controls. so it was a string then will be validated if it is a integer. here is my solution
public override bool Validate(Control control, object value)
{
if (value == null && !_IsAllowNull)
{
ErrorText = "Please provided valid number without a decimal point.";
return false;
}if (value.ToString().Contains(".")) { ErrorText = "Decimal value is not allowed"; return false; } if (!value.IsNumber()) { ErrorText = "Please provided valid number without a decimal point."; return false; } if (value.ToInt() < \_minValue || value.ToInt() > \_maxValue) { ErrorText = "Value should not be greater than " + \_maxValue + " or less than " + \_minValue; return false; } return true;}
the complexity of this solution just drop 3 point. but I like your solution, I will try to implement it. once again, thank you.
I'm glad you found some value in my response. While predicting the frequency of "correct" values entered by a user, or coming from some "other Control or code," is always dicey, I think it's often worth it. I can't know, looking at your code, if it's always the case that if the 'value parameter is an Int32 then the Control parameter will be null. If there is some variation in what you get in the Control parameter, you might be able to exploit that to simplify your code. If you think the Type of the 'value parameter is going to be Type Int32 more frequently than Type String, or some other Type:
if (value is Int32)
{
// convert to Int32 from object
int valasInt32 = Convert.ToInt32(val);// check for in range and return bool
return ?
}if (value is string)
{
// process string input
return ?
}// if you get here 'value is some other Type than String or Int32: try to Convert to Int32 ?
Ideally, you could use the suggestion by Chris Quinn on this thread to create an input Control that never let you enter any character that was not an integer. Does the 3rd. party Control you use (DevXpress ?) provide any Keyboard hooks you could exploit ?
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
-
I'm glad you found some value in my response. While predicting the frequency of "correct" values entered by a user, or coming from some "other Control or code," is always dicey, I think it's often worth it. I can't know, looking at your code, if it's always the case that if the 'value parameter is an Int32 then the Control parameter will be null. If there is some variation in what you get in the Control parameter, you might be able to exploit that to simplify your code. If you think the Type of the 'value parameter is going to be Type Int32 more frequently than Type String, or some other Type:
if (value is Int32)
{
// convert to Int32 from object
int valasInt32 = Convert.ToInt32(val);// check for in range and return bool
return ?
}if (value is string)
{
// process string input
return ?
}// if you get here 'value is some other Type than String or Int32: try to Convert to Int32 ?
Ideally, you could use the suggestion by Chris Quinn on this thread to create an input Control that never let you enter any character that was not an integer. Does the 3rd. party Control you use (DevXpress ?) provide any Keyboard hooks you could exploit ?
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
Yes it was a from Devexpres, actually i was implementing the ValidationRule from DevExpress.XtraEditors.DXErrorProvider then use it with dxValidationProvider here the code that will validate an int
public sealed class ValidateInteger : ValidationRule
{
private int _maxValue = int.MaxValue;
private int _minValue = int.MinValue;
private bool _IsAllowNull;public ValidateInteger(bool isAllowNull = false) { \_IsAllowNull = isAllowNull; } public ValidateInteger(int min, int max, bool isAllowNull = false) { \_maxValue = max; \_minValue = min; \_IsAllowNull = isAllowNull; } public override bool Validate(Control control, object value) { if (value == null && !\_IsAllowNull) { ErrorText = "Please provided valid number without a decimal point."; return false; } if (value.ToString().Contains(".")) { ErrorText = "Decimal value is not allowed"; return false; } if (!value.IsNumber()) { ErrorText = "Please provided valid number without a decimal point."; return false; } if (value.ToInt() < \_minValue || value.ToInt() > \_maxValue) { ErrorText = "Value should not be greater than " + \_maxValue + " or less than " + \_minValue; return false; } return true; }}
then use it like this
dxValidationProviderMain.SetValidationRule(txt_height, new Validators.ValidateInteger(false));
then call
dxValidationProviderMain.Validate()
-
Yes it was a from Devexpres, actually i was implementing the ValidationRule from DevExpress.XtraEditors.DXErrorProvider then use it with dxValidationProvider here the code that will validate an int
public sealed class ValidateInteger : ValidationRule
{
private int _maxValue = int.MaxValue;
private int _minValue = int.MinValue;
private bool _IsAllowNull;public ValidateInteger(bool isAllowNull = false) { \_IsAllowNull = isAllowNull; } public ValidateInteger(int min, int max, bool isAllowNull = false) { \_maxValue = max; \_minValue = min; \_IsAllowNull = isAllowNull; } public override bool Validate(Control control, object value) { if (value == null && !\_IsAllowNull) { ErrorText = "Please provided valid number without a decimal point."; return false; } if (value.ToString().Contains(".")) { ErrorText = "Decimal value is not allowed"; return false; } if (!value.IsNumber()) { ErrorText = "Please provided valid number without a decimal point."; return false; } if (value.ToInt() < \_minValue || value.ToInt() > \_maxValue) { ErrorText = "Value should not be greater than " + \_maxValue + " or less than " + \_minValue; return false; } return true; }}
then use it like this
dxValidationProviderMain.SetValidationRule(txt_height, new Validators.ValidateInteger(false));
then call
dxValidationProviderMain.Validate()
I don't use DevXpress, but any time I see a 'sealed Class, I'd wonder how I could modify it, since I can't sub-class it.
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
-
I don't use DevXpress, but any time I see a 'sealed Class, I'd wonder how I could modify it, since I can't sub-class it.
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
Yes, it was intentional sealed, i don't see any chances that it will be become a base class someday, but if it will then we can just remove the sealed keyword. I just don't know if it is a good practice.
-
Yes, it was intentional sealed, i don't see any chances that it will be become a base class someday, but if it will then we can just remove the sealed keyword. I just don't know if it is a good practice.
Well, yes, if you have the source, you could "un-seal" the class ... but ... I suggest you consider the idea that you may be pursuing "micro-optimization" here, and that may have very little pay-off. You might enjoy reading Eric Lippert's thoughts on micro-optimization in .NET: [^]
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
-
Well, yes, if you have the source, you could "un-seal" the class ... but ... I suggest you consider the idea that you may be pursuing "micro-optimization" here, and that may have very little pay-off. You might enjoy reading Eric Lippert's thoughts on micro-optimization in .NET: [^]
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
Bookmark I will gonna read it later. Thanks