1

I have a question regarding the best way to implement this. I'm going to describe my current implementation and how I seem to have painted myself into a corner:

I have an abstract class called Package:

public abstract class Package {
    protected String description;
    protected String packagingCode;
    protected Dimension dimensions;
    protected Weight weight;

    protected Package() {
        this.description = null;
        this.packagingCode = null;
        this.dimensions = null;
        this.weight = null;
    }

    protected Package(String description, String packagingCode, Dimension dimensions, Weight weight) throws ShippingException {
        this.description = description;
        this.packagingCode = packagingCode;
        this.dimensions = dimensions;
        this.weight = weight;

        String exceptionMessage = "";

        if(!meetsWeightRequirements()) {
            exceptionMessage = "This package's weight exceeds limits. ";
        }

        if(!meetsDimensionalRequirements()) {
            exceptionMessage += "This package's dimensions exceed limits.";
        }

        if(!StringUtils.isEmpty(exceptionMessage)) {
            throw new ShippingException(exceptionMessage);
        }
    }

    public String getDescription() {
        return description;
    }

    public void setDescription(String description) {
        this.description = description;
    }

    public String getPackagingCode() {
        return packagingCode;
    }

    public void setPackagingCode(String packagingCode) {
        this.packagingCode = packagingCode;
    }

    public Dimension getPackageDimensions() {
        return dimensions;
    }

    public void setPackageDimensions(Dimension dimensions) throws ShippingException {
        this.dimensions = dimensions;

        if(!meetsDimensionalRequirements()) {
            this.dimensions = null;
            throw new ShippingException("This package's dimensions exceed limits.");
        }
    }

    public Weight getPackageWeight() {
        return weight;
    }

    public void setPackageWeight(Weight weight) throws ShippingException {
        this.weight = weight;

        if(!meetsWeightRequirements()) {
            this.weight = null;
            throw new ShippingException("This package's weight exceeds limits.");
        }
    }


    public abstract boolean meetsWeightRequirements();

    public abstract boolean meetsDimensionalRequirements();
}

Then I have classes that extend this abstract class like so:

public class WeightBasedPackage extends Package {

    public boolean meetsWeightRequirements() {
        Weight weight = this.getPackageWeight();
        boolean meetsRequirements = false;

        if(weight != null) {
            meetsRequirements = (weight.getWeight() > 0);
        }

        return meetsRequirements;
    }

    public boolean meetsDimensionalRequirements() {
        return true;
    }
}

I have another object (ShipRequest) that maintains a List of Packages (List<Package>). I also have a services (eg WeightBasedPackageShipService) that uses this object and can access this list of packages. This implementation has worked fine because the services don't really care what type of package it is. The only difference between the packages is the way they implement the abstract methods.

Now here is where the problem comes in. I created a new class:

public class OrderQuantityPackage extends Package {

    int quantity;

    public OrderQuantityPackage() {
        super();
    }

    public void setQuantity(int quantity) {
        this.quantity = quantity;
    }

    public int getQuantity() {
        return this.quantity;
    }

    public boolean meetsWeightRequirements() {
        return true;
    }

    public boolean meetsDimensionalRequirements() {
        return true;
    }
}

Which has a quantity field. I need to access this field in the service (OrderQuantityPackageShipService). However, since it is of type Package I have to cast it (it seems kinda kludgey).

My question is, how do I implement this in a better fashion (so I don't have to cast) and also ensure type-safety (So that if you are using OrderQuantityPackageShipService, the package must be of type OrderQuantityPackage). I thought about using Generics, but it seems a little to kludgey for what I am trying to do (ShipRequest has a bunch of other attributes and it seemed strange to genericize it based on the type of package).

Thanks.

Vivin Paliath
  • 94,126
  • 40
  • 223
  • 295
  • Do I not understand your model correctly or should OrderQuantityPackage extend Package? – Will Marcouiller Mar 09 '10 at 17:24
  • My bad, that was a typo! – Vivin Paliath Mar 09 '10 at 17:26
  • I don't see why you couldn't cast your Package into OrderQuantityPackage, if it extends Package, into your OrderQuantityPackageShipService, as it should expect a quantity as per its name OrderQuantityPackageShipService. Am I missing something here? – Will Marcouiller Mar 09 '10 at 17:27
  • I've updated the question - I guess it is fine to cast, but I was wondering if there was a better way to approach this problem so that it doesn't require casting. – Vivin Paliath Mar 09 '10 at 17:37
  • As far as I'm concerned, it is good practice to go for the most general class when it is useful. However, in your situation, you seem to need a specific class to do the job depending on your Shipping Service. Then that said, I would opt for casting my base class and make sure it is of OrderQuantityPackage class and perhaps throwing when it is not. Otherwise, I would demand an instance of OrderQuantityPackage class as the input parameter for my service. – Will Marcouiller Mar 09 '10 at 17:38
  • "...when it gets casted to Package type initially, doesn't it lose the quantity field?" A Package object can reference instances of its subclasses, and in this case, the quantity won't get lost. – Kevin Le - Khnle Mar 09 '10 at 17:38
  • That's what I was thinking, but the problem is that I'm sending in a `ShipRequest` that has `List` inside it. I guess I could do an `instanceof` also to make sure that it is of the expected type. But I'm not to happy about that either :) I'm a fan of catching as many errors as you can at compile-time rather than run-time :) – Vivin Paliath Mar 09 '10 at 17:40
  • I now better understand your concern. I confirm that Khnle is right. Whether or not your class gets casted to Package, then casted back into its orginal derived type, the in-memory reference to your instance still persists, so when you will cast back to your derived type OrderQuantityPackage, you will gain back your Quantity property as it is a member of your class. – Will Marcouiller Mar 09 '10 at 17:42
  • @Will but wouldn't good design means there is no need for such kind of casts ? – sateesh Mar 09 '10 at 17:44
  • Then perhaps you should call the proper service depending on the declaring type of your Package item within your list. After a second thought, this shan't allow you to catch any error at compile-time. Well, this is such a situation where you can't do otherwise, I guess, or am I not enough qualified to push it further enough! ;P – Will Marcouiller Mar 09 '10 at 17:46
  • @sateesh: Agreed, but there are some times when you meet some particular situations where you have no other choice. In my experience, the simpler the better. Vivin has a good model after all. This is only a particular situation where casting will save him a lot of headaches trying to modelize the perfect model. The most important is that it works. Then, if he can afford the time to looking back at it later, then he should do so. Otherwise, he could declare his Quantity property into the base class and throw when it is not implemented/overrided. This would solve the cast approach, and is good. – Will Marcouiller Mar 09 '10 at 17:51
  • @Will, hmm looks like I would have to go the casting way :). I was also looking at http://stackoverflow.com/questions/358546/casting-between-arraylists-in-java and I wonder if I could apply that to my problem. – Vivin Paliath Mar 09 '10 at 18:11

4 Answers4

1
public abstract class Package {
    protected String description;  // These shouldn't be private fields instead of protected?
    protected String packagingCode; // Nah, I don't think so, otherwise how could I store a value into the Quantity field? =P 
    protected Dimension dimensions;  
    protected Weight weight;  
    protected int quantity;

    // Constructors, getters and setters...

    public virtual int getQuantity {
        throw new NotImplementedException();
    }

    public virtual int setQuantity(int quantity) {
        throw new NotImplementedException();
    }
}

public final class OrderQuantityPackage extends Package {
    public override int getQuantity {
        return super.quantity;
    }

    public override void setQuantity(int quantity) {
        super.quantity = quantity;
    }
}

I'm not completely sure about the syntax though, and neither about the NotImplementedException, but I hope you get the idea. So, any Package derived class that needs or require a quantity may do so by overriding the getter and setter of the Quantity property.

No exception should be thrown as of where the Quantity won't be required, it shouldn't get called, so no exception shall be thrown. Furthermore, it testifies that your model only does what it is required when times come.

In addition to it, OrderQuantityShipService shouldn't require a Weight property within the OrderQuantityPackage, and as written by Vivin, one could access the weight anyway.

Otherwise, a simple cast within your service should do it. It is no dirty way to go to use casting. For instance, one must cast the sender object within an event handler to the proper control type he wishes to check for name, state or other property values! The most general class is then passed on to the event, and one must cast... And this, that is not me who said to opt this way, these are software engineers!...

EDIT Vivin, how do one cast from a data type to another in JAVA, is it as in C/C++/C# ?

CastedType variable = (CastedType)TypeCast; 
Will Marcouiller
  • 23,773
  • 22
  • 96
  • 162
  • Thanks Will, let me try this method (I would have to convert it to Java - no virtual in Javaland) So yes, it means runtime errors vs compile-time, but that looks like the best I can do here. Also, thanks for that bit about casting. I guess it makes sense in certain cases - it's just that I've heard so many times that if you are casting, then you need to examine your model... which is what I'm trying to do :) And yes, those fields should be private. The only thing I'm not sure about is `quantity` being in the base class. I'd rather it stayed in the derived class :) – Vivin Paliath Mar 09 '10 at 18:33
  • LOL Thank's Vivin! I was not sure about this in Java. I have more experience in .NET, and wasn't sure about this specific use in JAVA. I'm growing more and more interested into JAVA as I easily recognize the code similar to C#, well, C# is similar to JAVA mostly, should I precise. =) Despite, both languages have taken some interesting features from each other here and there, so they both look pretty much the same. That is why I mention that I'm unsure about the exception in my code, etc. You're right about the casting thing, and you're a genius to interrogating yourself and consulting. – Will Marcouiller Mar 09 '10 at 18:39
  • Is there any reason why I lost my upvote, or is it SO's GUI that is tricking me? =P – Will Marcouiller Mar 09 '10 at 18:41
  • Hmm... I did give you an upvote - I don't know what happened (maybe I double clicked??). It's not letting me give you another one - it says it's too old :( Maybe if you edited it I would be able to give you one. Haha yeah, I've answered C# questions as well since they seem to be mostly similar to Java! I'm actually going to go the casting way, I've decided. Since the `OrderQuantityShipService` knows that it is getting a `OrderQuantityPackage`, I think it makes sense to cast it. Also, thanks for the compliment - just trying to make sure I'm writing good code :) – Vivin Paliath Mar 09 '10 at 18:52
  • Thanks for your concern about my upvote. After all, it's only a candy! =P I took the advantage of editing my answer by asking you a question for type casting in JAVA. =) – Will Marcouiller Mar 09 '10 at 19:05
  • Haha, yes it's the same as in Java. I'm doing: `OrderQuantityPackage orderQuantityPackage = (OrderQuantityPackage) myPackage;` – Vivin Paliath Mar 09 '10 at 19:22
0

Maybe you should create an abstract service and extend it for the different kinds of packages to handle. You could have the handling method be abstract and have each kind of service know what to do with the corresponding package. If you're not to mix types of packages then this might work.

pgmura
  • 708
  • 5
  • 16
0

Short Answer: Dependency Inversion

You have a OrderQuantityPackageShipService class that requires certain features from the objects that it processes. So OrderQuantityPackageShipService should be the one specifying those requirements. Typically this is done with an interface. If it is very specific to the service, create the interface nested. ie:

class OrderQuantityPackageShipService {
    //...
    interface QuantityPackage {
        int getQuantity();
        // ...
    }
}

if it can be used in a consistent manner by other services, define it outside of the OrderQuantityPackageShipService class.

Then have certain packages implement that interface...

tony
  • 3,737
  • 1
  • 17
  • 18
  • Is it not the same as having an OrderQuantityPackage derived from Package and specifying an additional property member? Doing so will only provide an object with the Quantity property. In order to make this work, you would need, I think, to implement this interface from Package directly. Making so is the same as making the Quantity property member of the Package base class and making it throw when not overrided, which in my humble point of view would rather be the solution to Vivin's approach, in addition to a good object model design. – Will Marcouiller Mar 09 '10 at 17:59
  • @Will: I'm not sure. And in a certain respect, that is the point of DI - I don't need or care to know most details. In my model, some packages will implement Quantity, some will not. If they all need to (because of some inherent characteristic of package) then yes, it should be in package (maybe default is to return 1). If it is not inherent, I'd keep it separate. YMMV. Hard to say from a general description, but I tend to push separation with interfaces more than most people, and it usually pays me back down the road (sometimes to far down the road to be worth the investment of course..) – tony Mar 09 '10 at 18:49
0

One thing I can think of is why would you need to access the quantity attribute in the class OrderQuantityPackageShipService ? As I look at it you have a getter and setter for each attribute of the class Package. Are these getters and setters really needed ? Having getters/setters for all those attributes doesn't go well with encapsulation.

Can you think of providing public methods in Package class that operate at a higher level and don't expose the internal attributes ? Wouldn't that help ?

sateesh
  • 27,947
  • 7
  • 36
  • 45