Interview question - what's wrong with this code?

C# Patterns Domain Driven Design (DDD) Dependency Injection (DI)

comments

2 minute read

Steve Wilkes, Principal Engineer

NB: This post is followed up here.

You can learn a lot about good code from reading bad code, and at least something about how well someone codes from what they can tell you about bad code. With this in mind I handed a print out of some bad code to the latest prospective developers I’ve interviewed, to see what they made of it. The bad code is below; I’ll write another blog with the things I think is wrong with it soon (there’s quite a lot of it), but it’d be very interesting to hear what people think is wrong with it before I do :)

The bad code - it doesn’t actually compile, but that’s the tip of its ‘badness’ iceberg…

namespace MyNamespace
{
    using System;

    public class Customer
    {

        public void PlaceOrder(string orderReference, OrderedProductData[] orderedProductData)
        {
 
            if (orderReference = "")
                throw new OrderCreationException();
 
            orderReference = orderReference.Trim();
 
 
            Order newOrder = new Order(orderReference);
 
            newOrder.Customer = this;
 
            int i;
 
            for (OrderedProductData dataItem in orderedProductData) {
                
                Product product = sqlserverdatabase.findproduct(dataItem.ProductID);
 
                newOrder.AddOrderItem(product);
 
                ++i;
            }
 
            LoggingService.LogInformation(
            "A new order has been placed: " + orderReference + " at " + DateTime.Now);
 
            CommunicationService.SendEmail(
                "New Order!",
                "A new order has been placed: " + orderReference + " at " + DateTime.Now,
                "[email protected]", "[email protected]");
        }
    }
}
sqlserverdatabase, LoggingService and CommunicationService are all classes which exist, but are not shown.

So what’s wrong with it? :)

Edit: I’ve had a couple of good responses to this, with things I hadn’t considered - I’ll approve all the comments when I post my take on it. Keep ‘em coming! :)

Edit 2: I’ve now posted the follow up to this.

Comments

5 comments

John Kraft • August 3rd, 2011
Here’s the ones I picked out in about 30 seconds…

Place order doesn’t belong on Customer. That’s like putting a Bake method on a Pizza. The Pizza doesn’t cook itself, the Oven cooks the Pizza. Same here. The OrderManager should take the orderReference, products, and customer as parameters.

orderReference is only checked against “” and not null or whitespace. Should read if(!string.IsNullOrWhitespace(orderReference))

Usage or newOrder and Product objects without ever checking to see if they are null.

varable i has no discernable purpose.

sqlserverdatabase, LoggingService and CommuncationServer should all be injected into the Customer class; and I don’t see anywhere that the sqlserverdatabase class is instantiated. I’m assuming the other two are static ‘global’ classes.

I’d prefer to use string.Format rather than appending the messages.

I don’t like the usings inside the namespace… but that’s stylistic.
anonymous • August 3rd, 2011
Null reference exception possible on both parameters specified.
What is i used for?
Alastair Smith • August 3rd, 2011
1. I’m just going to catch up-front all the various things like incorrect casing (e.g., sqlserverdatabase.findproduct) and Egyptian notation brackets under the heading “Style doesn’t match the coding guidelines published by MS.”
2. The guard clause for orderReference is assigning the value “” to the variable, not checking that it is “”. 2a. string.Empty should be used in place of “”. 2b. An Argument[Null]Exception should be thrown instead of an OrderCreationException().
3. orderReference is a string. This feels weakly-typed and I would argue it should be an instance of a separate class.
4. Much of the work completed in this method on Customer is actually the responsibility of an Order, thus the class violates the SRP.
5. Variable i is assigned but never used 5a. The loop should be a foreach, not a for.
6. The created Order’s Customer property is set outside the constructor, thus making the class mutable and allowing the Customer property to change. I can’t think of any reason why an Order would need to be assigned to a different Customer.
7. AddOrderItem is dealing directly with Products, when the PlaceOrder() method has an array of OrderedProductData. It would seem more sensible to make use of the OrderedProductData objects directly, because AddOrderItem() is losing any notion of quantities of products ordered.
8. The Order is never persisted via the sqlserverdatabase object. 8a. The sqlserverdatabase object is poorly-named, given that it seems to be functioning more like a Repository. Except FindProduct() is a static method.
9. How does the CommunicationService know where to send the email? It’s not clear which of the two of the provided email addresses, if either, is supposed to be the Customer’s, and if either of them are the Customer’s email address, then the address has been hard-coded and one Customer will receive notifications on all orders placed.
10. Use of string.Format() would tidy up the logging and SendEmail functions.
I just kept finding more stuff…!
Ryan • August 4th, 2011
Well aside from the fact that I’d probably use an OOB shopping cart (don’t reinvent the wheel and all…) there are quite a few changes I’d make.

Approaching this from a DDD perspective:
- You’re mixing aggregates in a way that probably doesn’t make sense. Customer and Order probably shouldn’t be directly connected so I’d have an OrderService with a method like:
  
  Order PlaceOrder(Customer, ShoppingCart);
- The Customer entity shouldn’t have data access logic in it.
- You shouldn’t use public setters to manipulate state. Wrap state transitions in methods like Order.CreateOrderFor(Customer, ReferenceId, Cart);
- orderReference.Trim() bothers me. If that’s really necessary couldn’t the Order entity do it? (Validate it’s inputs)
- I don’t see any transaction around creating an order. How do you rollback a failure?
- Getting the product entities causes a SELECT N+1 scenario. Also, why do you need the whole entity? Isn’t an id good enough to create a foreign key?
- Unused variable i;
- Logging and sending email could be aspects handled outside your entity.
- Short of an exception, PlaceOrder doesn’t provide any feedback to the caller.
Would I get the job? :D
quachnguyen • August 5th, 2011
Hello admin,

I think this line cause compiler issues

if (orderReference = “”)

And

newOrder.Customer = this;

Regards,

AgileObjects

Interview question - what's wrong with this code?

Comments

5 comments

Read Next

Self-Configuring Domain Event Handlers

Interview question - there's a lot wrong with this code :)

Comments

5 comments

Read Next

Tags