0
\$\begingroup\$

In the beginning of my methods that handle the calls from outside, I run this method to make sure that everything's OK, or let the caller know what's wrong:

private static void Validate(Guid userId, EntityState state, object data)
{
  if (userId == Guid.Empty)
    throw new Exception("Token unrecognized!");
  if (state == EntityState.Detached)
    throw new Exception("Action unrecognized!");
  if (data == null)
    throw new Exception("Data unrecognized!");
}

I was happy with the method until R# remarked that I'm declaring the variables without using them, except for pre-condition check. Well, it's true that I do. It's kind of the whole idea.

It bothers me that the tool remarks on it, because it feels like smarter people than me have put in the message there. However, turning it off isn't an option because that'd turn off even the checks for actually unnecessarily declared variables.

Should I refactor the code? How can I improve it?

\$\endgroup\$
  • 1
    \$\begingroup\$ You should consider using more helpful error messages. \$\endgroup\$ – 410_Gone Jan 11 '16 at 18:27
  • \$\begingroup\$ Check whether you pasted your real code correctly. Did you mean for the id parameter to be userId? Also, the question title should describe the task performed by the code (e.g. "Function to validate a GUID, an EntityState, and some data"). Alternatively, if you are asking a "What is best practice?" question about hypothetical/generic code rather than real concrete code, then it's not a Code Review, and would be better suited for Software Engineering. I think that your question could use a bit more contextual information about what these parameters represent and where their values come from. \$\endgroup\$ – 200_success Jan 11 '16 at 19:57
  • 1
    \$\begingroup\$ If you're asking a "What is best practice in these generic situations?" question, then it's not a question for Code Review. Shall we restore your original title and migrate it to Software Engineering? \$\endgroup\$ – 200_success Jan 11 '16 at 20:19
  • 2
    \$\begingroup\$ The question can remain on Code Review as is. However, it takes some imagination to figure out what you are trying to accomplish with this code. It could be improved if you added more code or information so that we can see why this function exists. What is EntityState? What is data? The contextual information would help us give you better advice, because there isn't much to work with. \$\endgroup\$ – 200_success Jan 11 '16 at 20:26
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Jan 12 '16 at 9:05
6
\$\begingroup\$

It makes sense to use these arguments and throw ArgumentException or ArgumentNullException instead of just Exception:

private static void Validate(Guid userId, EntityState state, object data)
{
    if (userId == Guid.Empty)
        throw new ArgumentException("Token unrecognized!", nameof(userId));
    if (state == EntityState.Detached)
        throw new ArgumentException("Action unrecognized!", nameof(state));
    if (data == null)
        throw new ArgumentNullException(nameof(data), "Data unrecognized!");
}

Since the nameof() is a C#6 feature, in the previous versions of C# you should use argument name string instead: "userId", "state", "data".

\$\endgroup\$
2
\$\begingroup\$

Besides Dmitry's suggestion and based on EBrown's comment, it makes sense to log more detailed information. This can be provided using a formatted exception string or using a logging library.

The second option has the advantage that can be customized to log only some levels (e.g. one usually wants less verbosity on production). The code using NLog (but this can be replaced with any other library or a custom one) would look like this:

private static void Validate(Guid userId, EntityState state, object data)
{
    String errorFormat = "{0} for user {1}, entity state {2}, object data = {3}";

    if (userId == Guid.Empty)
    {
        logger.Error(errorFormat, "Token unrecognized!", userId, state, data);
        throw new ArgumentException("Token unrecognized!", nameof(userId));
    }    

    if (state == EntityState.Detached)
    {
        logger.Error(errorFormat, "Action unrecognized!", userId, state, data);
        throw new ArgumentException("Action unrecognized!", nameof(state));
    }

    if (data == null)
    {
        logger.Error(errorFormat, "Data unrecognized!", userId, state, data);
        throw new ArgumentNullException(nameof(data), "Data unrecognized!");
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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