Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
public class NANDFunction : FunctionInfo
{
    public override string Name { get { return "NAND"; } }
    public override int MinArgs { get { return 2; } }
    public override int MaxArgs { get { return 2; } }

    public override object Evaluate(object[] args)
    {
        bool arg0 = CalcConvert.ToBool(args[0]);
        bool arg1 = CalcConvert.ToBool(args[1]);
        return ((arg0 != arg1) || (!arg0 && !arg1));
    }
}

What do you think of this code?

share|improve this question
    
Is FunctionInfo a class you've created? If so, what does it look like? –  Jeff Vanzella Jan 3 '13 at 19:20
    
no, it is from a third party control , a spread sheet control.. –  BDotA Jan 3 '13 at 19:22
    
static analysis tell me I can remove the last "!arg1" because it is always true...I don' get it why it says that either. –  BDotA Jan 3 '13 at 19:22
2  
Because if arg0 = true and arg1 = false then the left part of the or passes and the statement exists. Therefore if you get to the right part of the check, if arg0 is false the right part fails there, if it is true, it failed in the left side, so you will never hit the !arg1 check. –  Jeff Vanzella Jan 3 '13 at 19:39

1 Answer 1

up vote 4 down vote accepted

You may want to add some additional validation around how many arguments you're expecting (since you know via MinArgs and MaxArgs). You can also eliminate having to create an object array explicitly in the caller by using the params keyword (make sure you do it in the declaration in FunctionInfo as well). Lastly, if this class is intended to be non-inheritable (unlike FunctionInfo), mark it as sealed:

public sealed class NANDFunction : FunctionInfo
{
    public override string Name { get { return "NAND"; } }
    public override int MinArgs { get { return 2; } }
    public override int MaxArgs { get { return 2; } }

    public override object Evaluate(params object[] args)
    {
        if ((args.Length < this.MinArgs) || (args.Length > this.MaxArgs))
        {
            throw new ArgumentException("An insufficient number of arguments were passed.");
        }

        bool arg0 = CalcConvert.ToBool(args[0]);
        bool arg1 = CalcConvert.ToBool(args[1]);
        return (arg0 != arg1) || (!arg0 && !arg1);
    }   
}
share|improve this answer
2  
Now that I'm thinking about it, the argument validation (the if block with the throw) might be better handled in the FunctionInfo superclass' implementation of Evaluate and be replaced in the subclass with a call to base.Evaluate(args);. –  Jesse C. Slicer Jan 3 '13 at 21:33
1  
Sharing the common validation is a good idea, but I don't like calling the base implementation for that. For one, you're calling a method that returns something and you'll always ignore that. I think separating this into public Evaluate and protected abstract EvaluateNoValidation (or some better name) would be better. –  svick Jan 4 '13 at 14:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.