3
\$\begingroup\$

This method works correctly and verifies whether a user has permissions to view or edit the first parameter (value). I think it is not the best implementation, because there are too many conditions and the List object isn't necessary. How to improve it?

protected override Exception GetValidationException( object value, string name )
{
  List<Entity> entities = value is int ? new List<Entity>( 1 ) { EntityCache.Entity.GetEntity( (int) value ) } :
                          value is Guid ? new List<Entity>( 1 ) { EntityCache.Entity.GetEntityByGuid( (Guid) value ) } :
                          value is int[] ? (value as int[]).Select( i => EntityCache.Entity.GetEntity( i ) ).ToList() :
                          value is Entity ? new List<Entity>( 1 ) { value as Entity } :
                          value is Entity[] ? (value as Entity[]).ToList() :
                          null;

  if( entities == null )
    throw new ArgumentException( string.Format( RI.UnknownArgumentType, name, value, value.GetType().Name ) );

  bool hasPermission = (from one in entities
                        select EntityPermissionCache.PermissionChecker( one.Id, one.EntityOrganizationId ) &&
                               _checkEditInsteadOfView ? EntityTypePermissionCache.PermissionChecker( true, one.TypeId ) : true)
                       .All( a => a );

  return hasPermission ? null : new AccessViolationException( RI.AccessDenied );
}
\$\endgroup\$
4
  • \$\begingroup\$ Unless you call this method very often, I really doubt its performance is going to be a problem. Have you profiled your code? Is this method really the part that's slowing it down? If not, this is a premature optimization. \$\endgroup\$
    – svick
    Sep 16, 2013 at 10:42
  • \$\begingroup\$ Yes, it is a premature optimization. I have no way to test performance now, but this method calls everytime when users are trying to get any data from DB. \$\endgroup\$
    – FireShock
    Sep 16, 2013 at 10:53
  • 1
    \$\begingroup\$ This is not a comment about performance. Maybe you could rethink the design. It does not seem very clean to me to have so many ways to call entities (int, int[], Entity,...). If you do keep that many, maybe you could at least split your method into a first method to convert whatever to List<Entity> and then a second method to validate that list. \$\endgroup\$
    – toto2
    Sep 16, 2013 at 13:11
  • \$\begingroup\$ I agree. Splitting into different methods is good to understanding code. \$\endgroup\$
    – FireShock
    Sep 16, 2013 at 14:07

1 Answer 1

2
\$\begingroup\$

I think the assignment of your entities (bad name) is uber-abusing ternary expressions.

That said, my first thought was why on Earth would you want to return an Exception? So I googled up a bit and found this:

Is it valid for a function to return exception objects?

Absolutely. Here is a short list of examples where this may be appropriate:

  • An exception factory.
  • A context object that reports if there was a previous error as a ready to use exception.
  • A function that keeps a previously caught exception.
  • A third-party API that creates an exception of an inner type.

Fine then. But there's a flaw in your API - you're taking in an Object for what seems to be an Entity.

If I understand what you're trying to do here, I believe you'd be better off with an overloaded method:

protected override Exception GetValidationException(int value, string name)
protected override Exception GetValidationException(Guid value, string name)
protected override Exception GetValidationException(int[] value, string name)
protected override Exception GetValidationException(Entity value, string name)
protected override Exception GetValidationException(Entity[] value, string name)

...Or I'd find a way to make something like this work:

protected override Exception GetValidationException<TValue>(TValue value, string name)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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