5

We're working on a BugTracking system and are still in the learning process.

A BugReport has a title, description, (...) and also a tag. A tag represents the progess of the BugReport, e.g.

  • New : Freshly created BugReport
  • Under Review : A fix has been made and is under review.
  • Duplicate : The BugReport is a duplicate of another BugReport (i.e. it's about the same bug)

In the latter case, only one BugReport should be marked as a duplicate and it should store the BugReport of which it is a duplicate.

There's a use case where a BugReport should be able to be inspected : "The system shows a detailed overview of the selected bug report (...)"

Among peers, there's a discussion about which design is best.

Consider following two designs. Only relevant parts of the system is shown. The UI isn't necessarily part of the system.

Design 1

enter image description here

Pros:
In this design, the user receives an instance of IBugReport interface in which relevant inspectors are defined (getTitle(), getDescription(), ...). So, the user is able to retrieve only the information he requires.

Cons:
This design implies that every business object will need an interface that defines methods that may be used by the user. Also, when getDuplicate() is called on a BugTag, the user may retrieve a null from it if the BugTag is not a Duplicate. If, in the future, e.g. Under Review is required to have a field (e.g. a User field that indicates who is reviewing the BugReport), additional methods will have to be defined and implemented in all BugTag classes which most of them will return null, similarly to getDuplicate().

The peers who are in favor of this design have similar ideas for other design decisions i.e. when applicable, return an interface of a business object.

Design 2

enter image description here

[NOTE: in BugReport, inspectState() should be named inspectBugReport()]

Pros:
When BugTags gain additional fields, only inspectState() method will have to be changed accordingly. There's no need for interfaces for the business objects. the system does exactly what the use case asks: inspect the BugReport (i.e. return a String representing its representation).

Cons: The user gets a String in which all all information from the BugReport is concentrated. If the user only wanted the title, he will still receive the full BugReport information.

The peers who are in favor of this design have similar ideas for other design decisions i.e. parse the information to a String and expose that to the outside world.


We're a small group of three people in which myself came up with the second design and someone else with the first design. We're heavily discussing about this and we're both firmly standing our ground. The third person tries to evaluate both designs and give pros and cons.

We're trying to apply GRASP principles. Coupling and cohesion are very important principles in our design. Extensibility is important as well as the project undergoes several iterations. Each iteration requires additional functionality. Because of all this, I argue that the second design is much better. It is, for one, much more extensible.

What are additional pros and cons not mentioned above? Are both designs ok or are one/both designs not-done?

This question arose after this.

  • 1
    Can you make the title of your question more specific, please? It should summarize the question you're asking in the body of your post in 15 words or less. – Robert Harvey Mar 20 '16 at 15:53
  • 1
    Why not return an enum instead of a string for inspectedState? Also, interfaces should describe behavior. I don't see a benefit from introducing interfaces that are a one-to-one mapping to concrete classes. Just return the class. – RubberDuck Mar 20 '16 at 18:36
  • @RubberDuck "Why not return an enum instead of a string for inspectedState" In the case of Duplicate, inspectState() should reveal not only the state but also what field is kept there (duplicate : BugReport). "Just return the class" that would allow the user to mutate the object directly, which is not desirable. – Auberon Mar 20 '16 at 18:54
  • First of all, thank you for not calling it IBugTag. When I'm using it in BugReport it's nice not to even know that I'm talking to an interface. – candied_orange Mar 20 '16 at 19:34
3

Both of these designs violate: Tell, don't ask.

It may be too late. The rest of your design, or your mind set, might be demanding the ask way of doing things. If not, consider a world where you don't ask BugTag what's going on at all. You tell it when it's time to do it's thing.

BugTag


display(): void

This way, BugTag is a behavior object not a data object (like String). It will still hold data but the data will be encapsulated so no other objects need to know about it. You will need to pass BugTag something to talk to when it displays. But you really shouldn't feel like you are obligated to return any data to anybody. This is your data. Guard it jealously.

  • I like this approach best. This was my initial idea. However, display() would contain instructions such as System.out.println("something something");. Which I thought would create too much coupling with the System class. Or is coupling with the System class something we shouldn't worry about? The fact that it would be OS dependent is irrelevant in this project. – Auberon Mar 20 '16 at 19:38
  • 1
    Eeew! Why does BugTag.display() even know that we're talking to the console? Please, look up MVC and Observer Pattern. At the very least consider passing BugTag a ConsoleView object over a BugTag specialized View interface that lets BugTag tell WhateverView what to display (and not HOW to display). – candied_orange Mar 20 '16 at 19:46
  • I think I just had an epiphany, thank you for triggering it. Now I only need to convince my peers! – Auberon Mar 20 '16 at 20:16
1

The BugTag interface shouldn't even know Duplicate exists. In design 2 it doesn't know. It doesn't want to know. This is good. Having getDuplicate() in the BugTag in Design 1 creates a violation of the open/closed principle waiting to happen.

How, then, do you create a navigable link from one BugReport to another? Well, you do it like design 1. You just call it something that doesn't imply Duplicate exists.

BugTag


getLinkedBugReport(): BugReport
inspectState(): String

Now you have a design that won't break if later you have to add a Working BugTag or even a Revisiting BugTag that links to a BugReport.

1

Suggestion: Split out your Data Model/Entity Relationship Model away from your application's object model

In both scenarios it looks as if your data/entity model is mixed up with your application's behaviour.

From the raw data entities I see in your model, it looks as if they could be summarised as:

  • BugReport
  • Tag
  • Duplicate

From a raw data/entity perspective, there seems to be no real need for separate entities to represent New or Under Review tags; In your second diagram you have various Tag entities which seem to differ only by their stored data; making those entities redundant.

Furthermore, the entities above would certainly make sense in your Data Model/Data Access Layer, but don't seem to make sense as application objects (i.e. the objects which contain behaviour/methods relating to your functional requirements - mostly those surrounding your UI/Views).

Consider typical functions for a CRUD UI application

  • Read-only view for a list of partially visible data/abbreviated records
  • View data including calculated fields based on a database snapshot
  • Editor box to add/update new data
  • Preview changes before final commit (with the option to discard)
  • Validate newly typed data before enabling a button

The problem with tying your behaviour in to your entity model is that you not only make it harder to do a lot of typical CRUD UI operations, you also end up with those entity classes violating SRP.

Data Entities from a Data Model exist to represent the persistent/stored data for your application and nothing more. So - no validation, no UI/Display behaviour, no queries, no logic, just plain simple structured data.

Consider putting your complete data model (with raw data, no behaviour) into a DataModel class, which might include functions for your persistent store such as Load/Save, and accessors for the data itself.

Then using the data model for data access, you could encapsulate the rest of the application's behaviour into application objects, which are aware of your data model and it's structure, whose purpose among other things is to generate the View Data (a.k.a. "view models" to coin a popular UI term - where concerns such as UI validation might typically live; view models are application objects which are commonly used in patterns like MVC/MVP/MVVM to populate view data and to pass UI data downward to the main application).

for example:

// Possible ORM model:
class DataModel{
    public List<BugReport> reports;
    public List<Tag> tags;
    public List<Duplicate> duplicates;

    public void CreateReport(BugReport bugReport) { /* etc.. */ }
    public void CreateTag(Tag tag) { /* etc.. */ }
    public void Commit() { /* ... */ }
}

class BugReportMainApplication {
    private DataModel model;

    public BugReportMainApplication(DataModel dataModel) {
        model = dataModel;
    }

    public BugReportListViewModel GetReportList(List<Tag> queryTags) {
        // etc.
    }

    public EditorViewModel GetChangableReport(int id) {
        // etc.
    }

    public void AssignTag(int bugReportId, Tag tag) {
        // etc.
    }
}

Raw data entities generally shouldn't be interested in interfaces/polymorphism because they have no behaviour - so the issue regarding your getDuplicate() interface method would be irrelevant to the entity model; instead handled in whichever part of your application logic which deals specifically with displaying/handling your links to duplicate BugReports.

To summarise - consider splitting your application into Layers to avoid polluting the entities representing your raw data with application-specific logic, and to avoid being forced to write compromises/work arounds to imperfect abstractions such as your BugTag interface.

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.