4
\$\begingroup\$

I decided to build a basic eCommerce application in as a learning experience. In the past, I've gone with a fat controller, skinny model approach, and while it has worked, my results always end up messy and the code is/was kind of individual to the applications. To try something new, I wanted to incorporate the single responsibility principle (as much as possible) and group my application into service layers.

My current code can be found on Github.

Continuing with the actual architecture:

Entities: Base data-holder class.

Services: Handle all operations for a particular entity. Building, creating, updating, finding, and more.

Validators: Check entity data to ensure it meets any requirements. Interface is currently a single method, Validate(void * v). Implementation casts the passed in pointer to the appropriate entity.

int main(int argc, char ** argv)
{
    Pluto::Controller::AddSingleProduct * controllerAddSingleProduct = new Pluto::Controller::AddSingleProduct();
    controllerAddSingleProduct->ProcessRequest();

    return 0;
}

The ProcessRequest method called above:

void Pluto::Controller::AddSingleProduct::ProcessRequest()
{
    unsigned int productId = 1;
    std::string productName = "Test product.";
    unsigned int productPrice = 10;
    unsigned int productQuantity = 10;
    unsigned int productType = 1;
    unsigned int productStatus = 1;

    Pluto::Service::Product * productService = new Pluto::Service::Product();
    productService->SetProductValidator(new Pluto::Validator::Product());

    if (productService->Create(productService->Build(productId, productName, productPrice, productQuantity, productType, productStatus)))
    {
        this->ErrorView();
        return;
    }

    this->SuccessView();
}

The Pluto::Service::Product methods called above:

Pluto::Entity::Product * Pluto::Service::Product::Build(unsigned int id, std::string name, unsigned int price, unsigned int quantity, unsigned int type, unsigned int status)
{
    Pluto::Entity::Product * p = new Pluto::Entity::Product();
    p->SetId(id);
    p->SetName(name);
    p->SetPrice(price);
    p->SetQuantity(quantity);
    p->SetType(type);
    p->SetStatus(status);

    return p;
}

unsigned int Pluto::Service::Product::Create(Pluto::Entity::Product * product)
{
    if (product == NULL)
    {
        return 1;
    }

    if (this->productValidator)
    {
        if (this->productValidator->Validate(product))
        {
            return 1;
        }
    }

    return 0;
}

void Pluto::Service::Product::SetProductValidator(Pluto::Validator::Product * productValidator)
{
    this->productValidator = productValidator;
}

And finally, the Pluto::Validator::Product methods:

unsigned int Pluto::Validator::Product::Validate(void * v)
{
    Pluto::Entity::Product * p = (Pluto::Entity::Product *) v;

    if (p == NULL)
    {
    this->messageContainer->AddMessage
    (
        Pluto::Core::Message::TYPE_ERROR,
        Pluto::Validator::Product::ERROR_DEFAULT,
        "Invalid product.",
        (void *) p
    );

    return 1;
    }

    if (p->GetId() < 1)
    {
        this->messageContainer->AddMessage
        (
            Pluto::Core::Message::TYPE_ERROR,
            Pluto::Validator::Product::ERROR_ID,
            "Invalid ID.",
            (void *) p
        );
    }

    if (p->GetName().length() < 3)
    {
        this->messageContainer->AddMessage
        (
            Pluto::Core::Message::TYPE_ERROR,
            Pluto::Validator::Product::ERROR_NAME,
            "Invalid name.",
            (void *) p
        );
    }

    if (p->GetPrice() < 1)
    {
        this->messageContainer->AddMessage
        (
            Pluto::Core::Message::TYPE_ERROR,
            Pluto::Validator::Product::ERROR_PRICE,
            "Invalid price.",
            (void *) p
        );
    }

    if (p->GetQuantity() < 1)
    {
        this->messageContainer->AddMessage
        (
            Pluto::Core::Message::TYPE_ERROR,
            Pluto::Validator::Product::ERROR_QUANTITY,
            "Invalid quantity.",
            (void *) p
        );
    }

    if (p->GetType() < 0)
    {
        this->messageContainer->AddMessage
        (
            Pluto::Core::Message::TYPE_ERROR,
            Pluto::Validator::Product::ERROR_TYPE,
            "Invalid type.",
            (void *) p
        );
    }

    if (p->GetStatus() < 0)
    {
        this->messageContainer->AddMessage
        (
            Pluto::Core::Message::TYPE_ERROR,
            Pluto::Validator::Product::ERROR_STATUS,
            "Invalid status.",
            (void *) p
        );
    }

    if (this->messageContainer->GetNumMessages())
    {
        return 1;
    }

    return 0;
}

The messageContainer within Pluto::Validator::Product, and all validators, for that matter is a very basic array-like class. Calling AddMessage places the data passed in at the current position and then increments. The details of it can be found in the repository linked above.

I feel my controllers are a lot more maintainable and manageable like this. The product service seems flexible in that I can switch the validator as necessary to test or deal with different validation rules by subclassing. I'm also comfortable with how the validator class worked out. It's really basic, but it seems isolated and only handles its one job.

The problem I'm running into is, how do I get messages/warnings/errors/etc back out to the controller efficiently? The message container class seen above was an attempt to solve this problem. It works great if I instantiate the validator in the controller and use it, then utilize the message container it provides in the views. I'm having trouble seeing a good way to do this in the product service, and then passing the data back out to the controller.

Below is a quick example of how I might have done validation before:

void AddProductController()
{
    Pluto::Entity::Product * p = new Pluto::Entity::Product();
    p->SetId(1);
    p->SetName("Example Product");
    // Continue populating product as before.

    unsigned int validationErrors = 0;
    unsigned int maxValidationErrors = 5;
    unsigned int numValidationErrors = 0;
    std::string errorStrings[maxValidationErrors];

    if (p->GetId() < 1)
    {
        validationErrors = 1;
        errorStrings[numValidationErrors] = "The product ID was invalid.";
        numValidationErrors++;
    }

    if (p->GetName().length() < 3)
    {
        validationErrors = 1;
        errorStrings[numValidationErrors] = "The product name was too short.";
        numValidationErrors++;
    }

    // Continue checking the product data.

    if (validationErrors)
    {
        for (unsigned int i = 0; i < numValidationErrors; i++)
        {
            std::cout << "Error: " << errorStrings[i] << "\n";
        }

        return;
    }

    std::cout << "Product was validated successfully!\n";
}

Worth noting, the service might have other things it does as well that need to spit back messages or errors to the client. For example, persisting the product on disk somehow, which could succeed or fail for any number of reasons. In the future, I may want to expand my views to use HTML or some type of layout as well, so it seems I'd need to be able to detect certain kinds of messages I'm expecting for positioning.

Am I correct in thinking that my approach to the services and validators is worthwhile? Any suggestions on how to handle the message/warning/error issue discussed above is really appreciated. It's something that's holding me back right now.

\$\endgroup\$
3
+50
\$\begingroup\$

I would agree that what you have now is cleaner and more testable (in comparison to the code you said you might have written before).

I don't know C++ but I'm familiar with validation, so I'll stick to that.

  • When adding a Product, I think it's best not to explicity set an ID. I'm assuming these entries are going to a database of some sorts (relational, key, or anything else). If this is the case, let the database set the ID, or let it tell you what ID comes up next.

  • I think it's limiting to have restrictions on name length. I could see an eCommerce store selling an ax. I'm not sure why you'd limit one and two letter words (why not three or four letter words);just check if there's a name at all.

  • Similar principle with the price. Supposedly it can't be less than 1. What if the store offers a holiday sale and the $1 item is 50% off, and the $0.25 is free! I'm just thinking of all the possible ways you could run into problems down the road.

  • Also, you set price as an int. This is fine, except now there's no cents. I suggest another variable handling the cents (also an int, limited to two digits).

\$\endgroup\$
  • \$\begingroup\$ Alex, thanks for the response. It has been quite some time since I wrote this code and have improved a number of things since then. The ID is set explicitly on creation just for initialization sake. It isn't used and is automatically set by the mapping layer. The name length restriction is a minimum and could be changed, i.e. it's a placeholder. I'd handle discounts elsewhere, as that is more of a per-application concept. Price should definitely be an integer. Instead of storing 1.00, 100 is stored, so the cent information is maintained. Formatting the price is view-related. \$\endgroup\$ – ChickenFarmer Jan 10 '16 at 17:21
  • \$\begingroup\$ Maybe my post was of no use, and if I could, I'd return the rep I received from the bounty (which I'd be fine with, I'd rather have my post be helpful than get the rep.) I can't believe I missed the integer for price thing. That's the way to go and I use that method myself anyways. \$\endgroup\$ – Alex L Jan 12 '16 at 4:42

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.