0
\$\begingroup\$

I'm trying to determine at which level I should validate a method on my class. In my class method I throw an exception if it ever gets to that point. In my service layer I do some validation and return a validation object to be used by the controller. Here is my code. Does it seem like I am moving in the right direction? Let me know if you need any additional code.

// Class Method
public void Delete()
{
    if (Status == "UnIndexed")
        Deleted = true;
    else
        throw new InvalidOperationException("Only an UnIndexed thing may be deleted.");
}

// Service Layer
public IValidator<Thing> RemoveThing(int id)
{
    var thing = _repository.Get(id);
    if (thing == null)
        return null;

    var validator = new ThingValidator(thing);
    var isValid = validator.Validate(new StatusSpecification("UnIndexed"));

    if (isValid)
    {
        thing.Delete();
        _repository.Save(thing);
    }
    else
    {
        validator.AddError("Only an UnIndexed thing may be deleted.");
    }

    return validator;
}

// Controller
public JsonResult DeleteThing(int thingId)
{
    var command = _service.RemoveThing(thingId);

    if (command == null)
        return Json(HttpStatusCode.NotFound);

    if (command.IsValid())
    {
        return Json(HttpStatusCode.OK);
    }
    else
    {
        return Json(command.ListErrors());
    }
}
\$\endgroup\$

closed as off-topic by t3chb0t, Toby Speight, Graipher, alecxe, Mast Oct 16 '17 at 6:37

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Is there a reason you didn't go with the standard IValidatableObject? the ASP.NET MVC framework knows about this interface and is plumbed to work with it. \$\endgroup\$ – CharlesNRice Oct 11 '17 at 14:02
  • \$\begingroup\$ Is IValidateObject useful only at the MVC level or is it recommended to use it in service layers? My MVC project won't be the only front end that hits my service layer. \$\endgroup\$ – DrivenTooFar Oct 11 '17 at 14:15
  • \$\begingroup\$ You add IValidateObject to your class and have it return ValidationResults. MVC will auto call it from model binding. If you want to manually call it you can use the Validator class \$\endgroup\$ – CharlesNRice Oct 11 '17 at 14:17
2
\$\begingroup\$

Some things that stuck out to me. FWIW.

//NOTE: for me, this validator can only describe what went wrong without
//        giving you a specific way to handle each kind of thing that happened.
//        e.g.  NULL (translates to) NotFound
//        e.g.  AddError(<string>) can only be interpreted as "other" errors since they are just descriptions of the error and not the type
public IValidator<Thing> RemoveThing(int id)
{    
    var thing = _repository.Get(id);
    // NOTE: this result has to be interpreted by the caller
    if (thing == null)
        return null; 

    var validator = new ThingValidator(thing);
    var isValid = validator.Validate(new StatusSpecification("UnIndexed"));    

    if (isValid)
    {
        thing.Delete();  
        _repository.Save(thing);
    }
    else
    {
        // NOTE: since this message comes from outside of the class, it
        //         it not the nearest source of truth about what went wrong.
        //         i think this message should come from the class itself.
        //          this message is also typed twice in your code.
        validator.AddError("Only an UnIndexed thing may be deleted.");
    }

    return validator;
}

public JsonResult DeleteThing(int thingId)
{
    var command = _service.RemoveThing(thingId);

    // NOTE: this could be set up so that you don't have to interpret null as "not found"
    if (command == null)
        return Json(HttpStatusCode.NotFound);

    if (command.IsValid())
    {
        return Json(HttpStatusCode.OK);
    }
    else
    {
        return Json(command.ListErrors());
    }
}

OPTION #1

  • short and to the point.
  • idiomatic solution
  • very little logic.
  • explicitly typed errors for try/catch handling

THROW ALL THE EXCEPTIONS!!!

public void Delete()
{
    if (Status == "UnIndexed")
        Deleted = true;
    else
        throw new UnIndexedException();
}

public void RemoveThing(int id)
{    
    var thing = _repository.Get(id);
    if (thing == null)
        throw new ThingNotFoundException(id); 

    thing.Delete();  
    _repository.Save(thing);    
}

// Controller
public JsonResult DeleteThing(int thingId)
{
    try
    {
        _service.RemoveThing(thingId);
        return Json(HttpStatusCode.OK);
    }
    catch (ThingNotFoundException e)
    {
        return Json(HttpStatusCode.NotFound);
    }    
    catch (Exception e)
    {
        throw;
    }
}

OPTION #2

  • less short but still to the point.
  • inching towards a more declarative solution. (which is good)
  • not that much logic.
  • explicitly stated errors for if/then handling

NEVER THROW EXCEPTIONS!!!

class RemoveThingResult
{
    private RemoveThingResult(bool isOk, boo isNotFound, bool isUnIndexed)
    {
        IsOk = isOk; IsNotFound = isNotFound; IsUnIndexed = isUnIndexed;
    }

    public readonly bool IsOk;
    public readonly bool IsNotFound;
    public readonly bool IsUnIndexed;

    public static RemoveThingResult Ok()
    {
        return new RemoveThingResult(true, false, false);
    }
    public static RemoveThingResult NotFound()
    {
        return new RemoveThingResult(false, true, false);
    }
    public static RemoveThingResult UnIndexed()
    {
        return new RemoveThingResult(false, false, true);
    }
}

// Service Layer
public RemoveThingResult RemoveThing(int id)
{    
    var thing = _repository.Get(id);    

    if (thing == null)
        return RemoveThingResult.NoFound(); 

    var validator = new ThingValidator(thing);
    if (!validator.Validate(new StatusSpecification("UnIndexed")))
        return RemoveThingResult.UnIndexed(); 

    thing.Delete();  
    _repository.Save(thing);

    return RemoveThingResult.Ok(); 
}

// Controller
public JsonResult DeleteThing(int thingId)
{
    var command = _service.RemoveThing(thingId);

    if (command.IsNotFound)
        return Json(HttpStatusCode.NotFound);

    if (command.IsOk)    
        return Json(HttpStatusCode.OK);

    if (command.IsUnIndexed)    
        return Json("Only an UnIndexed thing may be deleted.");

}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback! In your opinion, which option would you choose if you were doing something similar? \$\endgroup\$ – DrivenTooFar Oct 12 '17 at 14:23
  • 1
    \$\begingroup\$ @DrivenTooFar I would do something further than the "never throw" solution. I'm into functional programming (even in C#) at present where creating more types and workflows are more explicit. I would use a result monad or something of the sort. I didn't go that far in the example, because seeing functional stuff in C# can distract from the point. \$\endgroup\$ – TBone Oct 12 '17 at 15:41

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