Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm starting a school group project in Java, using Swing. It's a straightforward GUI on Database desktop app.

The professor gave us the code from last year's project so we could see how he does things. My initial impression is that the code is far more complicated than it ought to be, but I imagine programmers often think this when looking at code they didn't just write.

I'm hoping to find reasons why his system is good or bad. (I asked the professor and he said I'd see later on why it's better, which doesn't satisfy me.)

Basically, to avoid any coupling between his persistable objects, models (business logic), and views, everything is done with strings. The persistable objects that get stored in the database are hashtables of strings, and the models and views "subscribe" to each other, providing string keys for the "events" they subscribe to.

When an event is triggered, the view or model sends a string to all of its subscribers who decide what to do for that event. For instance, in one of the views action listener methods (I believe this just sets the bicycleMakeField on the persistable object):

    else if(evt.getSource() == bicycleMakeField)
    {
        myRegistry.updateSubscribers("BicycleMake", bicycleMakeField.getText());
    }

That call eventually gets to this method in the Vehicle model:

public void stateChangeRequest(String key, Object value) {

            ... bunch of else ifs ...

    else
    if (key.equals("BicycleMake") == true)
    {
                ... do stuff ...

The professor says that this way of doing stuff is more extensible and maintainable than having the view simply call a method on a business logic object. He says there is no coupling between the views and models because they don't know of each others' existence.

I think this is a worse kind of coupling, because the view and model have to use the same strings in order to work. If you remove a view or model, or make a typo in a string, you get no compile errors. It also makes the code a whole lot longer than I think it needs to be.

I want to discuss this with him, but he uses his industry experience to counter any argument that I, the inexperienced student, might make. Is there some advantage to his approach that I'm missing?


To clarify, I want to compare the above approach, with having the view be obviously coupled to the model. For instance, you could pass the vehicle model object to the view, and then to change the "make" of the vehicle, do this:

vehicle.make = bicycleMakeField.getText();

This would reduce 15 lines of code that are currently ONLY used to set the make of the vehicle in one place, to a single readable line of code. (And since this kind of operation is done hundreds of times throughout the app, I think it would be a big win for readability as well as safety.)


Update

My team leader and I restructured the framework the way we wanted to do it using static typing, informed the professor, and ended up giving him a demo. He's generous enough to let us use our framework as long as we don't ask him for help, and if we can keep the rest of our team up to speed -- which seems fair to us.

share|improve this question
10  
OT. I think your professor should update himself and not use old code. In the academia there is no reason to use a JAVA version from 2006 when it's 2012. –  Farmor Jan 31 '12 at 15:09
3  
Please keep us posted when you eventually get his answer. –  JeffO Jan 31 '12 at 15:40
4  
Regardless of the quality of the code or the wisdom of the technique, you should drop the word "magic" from your description of the strings used as identifiers. "Magic strings" implies that the system isn't logical, and it's going to color your view and poison any discussion you have with your instructor. Words like "keys," "names," or "identifiers" are more neutral, perhaps more descriptive, and more likely to result in a helpful conversation. –  William Shakespeare Jan 31 '12 at 15:46
1  
True. I haven't called them magic strings while talking to him, but you're right, there's no need to make it sound worse than it is. –  Philip Jan 31 '12 at 15:49
2  
The approach your professor describes (notifying listeners via events named as strings) can be good. Actually enums could make more sense, but this is not the biggest problem. The real issue I see here is that the object receiving the notification is the model itself, and then you have to manually dispatch based on the kind of event. In a language where functions are first class you would register a function, not a n object to an event. In Java, I guess one should use some kind of object wrapping a single function –  Andrea Jan 31 '12 at 16:29

8 Answers 8

The approach your professor proposes is best described as stringly typed and is wrong on almost every level.

Coupling is best reduced by dependency inversion, which does so by a polymorphic dispatch, rather than hardwiring a number of cases.

share|improve this answer
3  
You write "almost every level", but you have not written why it's not (in your opinion) a suitable case in this example. Would be interesting to know. –  hakre Feb 1 '12 at 15:39
1  
@hakre: There are no suitable cases, at least not in Java. I said, it "is wrong on almost every level", because it is better than using no indirection at all and just throwing all the code in one place. However using (global) magic string constants as a basis for a manual dispatch is just a convoluted way of using global objects in the end. –  back2dos Feb 2 '12 at 5:51
    
So your argument is: There are never suitable cases, so this one ain't one, too? That's hardly an argument and actually not very helpful. –  hakre Feb 2 '12 at 6:08
1  
@hakre: My argument is, that this approach is bad for a number of reasons (paragraph 1 - enough reasons to avoid this are given in the explanation of stringly typed) and shouldn't be used, because there's a better one (paragraph 2). –  back2dos Feb 2 '12 at 6:49
3  
@hakre I can imagine a situation that would warrant this approach would be if a serial killer had duct-taped you to your chair and was holding a chainsaw over your head, laughing manically, and ordering you to use string literals everywhere for dispatch logic. Short of that, relying on language features to do this sort of things is preferable. Actually, I can think of one other case involving crocodiles and ninjas, but that's a little more involved. –  Rob Y Sep 26 at 22:28

Your professor is doing it wrong. He's trying to completely decouple the view and model by forcing communication to travel via string in both directions (view doesn't depend on model and model doesn't depend on view), which totally defeats the purpose of object-oriented design and MVC. It sounds like instead of writing classes and designing an OO-based architecture, he's writing disparate modules that simply respond to text-based messages.

To be somewhat fair to the professor, he is trying to create in Java what looks very similar to the highly common .NET interface called INotifyPropertyChanged, which notifies subscribers of change events by means of passing strings that specify which property has changed. In .NET at least, this interface is primarily used to communicate from a data model to view objects and GUI elements (binding).

If done right, taking this approach can have some benefits¹:

  1. The View depends on the Model, but the Model does not need to depend on the View. This is appropriate Inversion of Control.
  2. You have a single place in your View code that manages responding to change notifications from the model, and only one place to subscribe/unsubscribe, reducing the likelihood of a hanging reference memory leak.
  3. There is way less coding overhead when you want to add or remove an event from the event publisher. In .NET, there is a built-in publish/subscribe mechanism called events but in Java you would have to create classes or objects and write subscribe/unsubscribe code for each event.
  4. The biggest downfall with this approach is that the subscribing code can fall out of sync with the publishing code. However, this is also a benefit in some development environments. If a new event is developed in the model and the view is not yet updated to subscribe to it, then the view will still likely work. Likewise, if an event is removed, you have some dead code, but it could still compile and run.
  5. In .NET at least, Reflection is used very effectively here to automatically call getters and setters depending on the string that was passed to the subscriber.

So, in short (and to answer your question), it can be done, but the approach your professor is taking is at fault for not allowing at least a one-way dependency between the View and Model. It's just not practical, overly academic, and not a good example of how to use the tools at hand.


¹ Search Google for INotifyPropertyChanged to find a million ways people find to try to avoid using magic string when implementing it.

share|improve this answer
    
@MainMa thanks for the edit! :-) –  Kevin McCormick Jan 31 '12 at 20:09
    
It sounds like you described SOAP here. :D … (but yes, I get your point and you’re right) –  Konrad Rudolph Jan 31 '12 at 21:16

enums (or public static final Strings) should be used instead of magic Strings.

Your professor doesn't understand OO. For example having a concrete Vehicle class?
And to have this class understand the make of a bicycle?

Vehicle should be abstract and there should be an concrete subclass called Bike. I would play by his rules to get a good grade and then forget his teaching.

share|improve this answer
3  
Unless the point of this exercise is to improve the code using better OO principles. In that case, refactor away. –  joshin4colours Jan 31 '12 at 15:57
2  
@joshin4colours If StackExchange was grading it you would be right; but since the grader is the same person who came up with the idea he'd give all of us bad marks because he knows his implementation was better. –  Dan Neely Jan 31 '12 at 18:32

I think you have a good point, the magic strings are bad. Someone on my team had to debug a problem caused by magic strings a couple weeks ago (but it was in their own code that they wrote so I kept my mouth shut). And it happened... in industry!!

The advantage to his approach is that it might be faster to code, especially for small demo projects. Disadvantages are that it is prone to typos an the more often the string is used the more chance a typo code screw things up. And if you ever want to change a magic string, refactoring strings is harder than refactoring variables since refactoring tools might also touch strings that contain the magic string (as a substring) but are not themselves the magic string, and should not be touched. Also, you might need to keep a dictionary or index of magic strings so new developers won't start inventing new magic strings for the same purpose, and won't waste time looking up existing magic strings.

In this context, it looks like an Enum might be better than magic strings or even global string constants. Also, IDEs will often give you some sort of code-assist (auto-complete, refactor, find-all-references, etc...) when you use constant strings or Enums. An IDE won't offer much help if you use magic strings.

share|improve this answer
    
I agree that enums are better than string literals. But even then, is it really better to call a "stateChangeRequest" with an enum key, or just call a method on the model directly? Either way, the model and view are coupled in that one spot. –  Philip Jan 31 '12 at 16:09
    
@Philip: Well then, I guess to decouple the model and view at that point, you might need some sort of controller-class to do that... –  FrustratedWithFormsDesigner Jan 31 '12 at 16:13
    
@FrustratedWithFormsDesigner - Yes. An MVC architecture is the most decoupled –  cdeszaq Jan 31 '12 at 16:17
    
Right, if another layer will help decouple them, I wouldn't argue against that. But that layer could still be statically typed and use actual methods rather than string or enum messages to connect them. –  Philip Jan 31 '12 at 16:22

Using 'industry experience' is a poor argument. Your professor needs to come up with a better argument than that :-).

As others have stated, the use of magic Strings is certainly frowned upon, using enums is considered far safer. An enum has meaning and scope, magic strings do not. Aside from that, the manner in which your professor is decoupling concerns is considered to be an older and more fragile technique than used today.

I see that @backtodos has covered this whilst I was answering this, so I'll simply add my opinion that using Inversion of Control (of which Dependency Injection is a form, you'll run across the Spring framework in industry before to long...) is considered a more modern way to deal with this type of decoupling.

share|improve this answer
2  
Fully agree academia should have far higher requirements than the industry. The academia == best theoretical way; The industry == best practical way –  Farmor Jan 31 '12 at 15:19
3  
Unfortunately, some of those who teach this stuff in academia do so because they can't cut it in industry. On the positive side, some of those in academia are brilliant and should not be kept hidden away in some corporate office. –  FrustratedWithFormsDesigner Jan 31 '12 at 15:20

Let's be quite clear; there is inevitably some coupling in there. The fact that there is such a subscribable topic is a point of coupling, as is the nature of the rest of the message (such as “single string”). Given that, the question then becomes one of whether the coupling is greater when done via strings or types. The answer to that depends on whether you're concerned about coupling that is distributed over time or space: if the messages are going to be preserved in a file before being read in later, or are going to be sent to another process (especially if that's not written in the same language), using strings can make things much simpler. The downside is that there is no type checking; blunders won't be detected until runtime (and sometimes not even then).

A mitigation/compromise strategy for Java can be to use a typed interface, but where that typed interface is in a separate package. It should consist of just Java interfaces and basic value types (e.g., enumerations, exceptions). There should be no implementations of interfaces in that package. Then everyone implements against that and, if conveying the messages in a complex way is necessary, a particular implementation of a Java delegate can use magic strings as necessary. It also makes it much easier to migrate the code to a more professional environment such as JEE or OSGi, and greatly assists little things like testing…

share|improve this answer

What your professor is proposing is the use of "magic strings". While it does "loosely couple" classes between each other, allowing for easier changes, it is an anti-pattern; strings should very, VERY rarely be used to contain or control code instructions, and when it absolutely has to happen, the value of the strings you are using to control logic should be centrally and constantly defined so there is ONE authority on the expected value of any particular string.

The reason why is very simple; strings are not compiler-checked for syntax or consistency with other values (at best, they're checked for proper form regarding escape characters and other language-specific formatting within the string). You can put anything you want in a string. As such, you can get a hopelessly broken algorithm/program to compile, and it's not until it runs that an error occurs. Consider the following code (C#):

private Dictionary<string, Func<string>> methods;

private void InitDictionary() //called elsewhere
{
   methods = new Dictionary<string, Func<string>> 
      {{"Method1", ()=>doSomething()},
       {"MEthod2", ()=>doSomethingElse()}} //oops, fat-fingered it
}

public string RunMethod(string methodName)
{
   //very naive, but even a check for the key would generate a runtime error, 
   //not a compile-time one.
   return methods[methodName](); 
}

...

//this is what we thought we entered back in InitDictionary for this method...
var result = RunMethod("Method2"); //error; no such key

... all of this code compiles, first try, but the error is obvious with a second look. There are numerous examples of this kind of programming, and they are all subject to this kind of error, EVEN IF you define strings as constants (because constants in .NET are written to the manifest of each library in which they are used, which must then all be recompiled when the defined value is changed in order for the value to change "globally"). As the error is not caught at compile-time, it must be caught during run-time testing, and that's only guaranteed with 100% code coverage in unit tests with adequate code exercise, which is usually only found in the most absolute fail-safe, real-time systems.

share|improve this answer

Actually, those classes are still tightly coupled. Except now they're tightly coupled in a such a way that the compiler can't tell you when things are broken!

If someone changes "BicycleMake" to "BicyclesMake" then no one is going to find out that things are broken until runtime.

Runtime errors are way more costly to fix than compile time errors - this is just all kinds of evil.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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