I need help improving my code

[John Torjo]

I certainly beg to differ. The code is much more readable. But please, provide an alternative :) You may be right :) Best, John

-- LogWizard Meet the Log Viewer that makes monitoring log files a joy!

[V 0]

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)

[V 0]

It's probably a matter of taste. ;) I personally hate the lack of brackets, because it makes it hard to insert a statement if the code needs modification, but in the end it's up to the developer. You did remove the multiple exit points :thumbsup:.

V.
(MQOTD rules and previous solutions)

[John Torjo]

Thanks :) The lack of brackets is taste-dependent for sure :) I like to make code as short as possible without sacrificing readability. Having said that, about code needing modification - resharper makes it extremely easy to do. Namely , you open the bracket {, go after the statement, and it will write the closing bracket for you. Best, John

-- LogWizard Meet the Log Viewer that makes monitoring log files a joy!

[Chris Quinn]

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. =========================================================

[Rob Philpott]

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.

[Gilbert Consellado]

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.

[Gilbert Consellado]

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

[Gilbert Consellado]

Yes I can do this. but i can easily miss it. thanks

[Gilbert Consellado]

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

[BillWoodruff]

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.

[Gilbert Consellado]

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()

[BillWoodruff]

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.

[Gilbert Consellado]

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.

[BillWoodruff]

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.

[Gilbert Consellado]

Bookmark I will gonna read it later. Thanks