6
\$\begingroup\$

I have been searching for a solution to show data of class with three assumptions:

  1. Class Employee is not responsible for showing itself
  2. Class Employee does not expose its data with getters
  3. Classes responsible for showing data are immutable

So I created Employee class with its View interface:

public final class Employee {

  private String name;
  private String salary;

  public interface View {
    View addData(String name, String value);
    void show();
  }

  public void showOn(View view) {
    view.addData("Name", name.toString())
        .addData("Salary", salary.toString())
        .show();
  }
}

And two classes to show it to user. HTML:

final class HtmlPage implements Employee.View {
  private final String content;

  public HtmlPage() {
    content = "";
  }

  public HtmlPage(String str) {
    content = str;
  }

  @Override
  public Employee.View addData(String name, String value) {
    String newData = "\t<tr><td>";
    newData += name;
    newData += "</td><td>";
    newData += value;
    newData += "</td></tr>\n";

    HtmlPage updated = new HtmlPage(content + newData);
    return updated;
  }

  @Override
  public void show() {
    System.out.println("<table>\n" + content + "</table>");
  }
}

and GUI:

import javax.swing.*;
import java.awt.*;
class OutputWindow implements Employee.View {

  private final JComponent panel;

  public OutputWindow() {
    this.panel = new JPanel();
  }

  public OutputWindow(JComponent panel) {
    this.panel = panel;
  }

  @Override
  public Employee.View addData(String name, String value) {
    panel.add(new JLabel(name));
    panel.add(new JLabel(value));

    return this;
  }

  @Override
  public void show() {
    JFrame outputFrame = new JFrame("Okienko do wypisania danych");
    outputFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    outputFrame.getContentPane().add(this.getJComponent());
    outputFrame.pack();
    outputFrame.show();
  }

  JComponent getJComponent() {
    panel.setLayout(new GridLayout(3, 2));
    return panel;
  }
}

Here is the usage:

public static void main(String[] args) {
    Employee Piotr = new Employee();

    OutputWindow outputWindow = new OutputWindow();
    Piotr.showOn(outputWindow);

    Employee Ewelina = new Employee();

    HtmlPage htmlPage = new HtmlPage();
    Ewelina.showOn(htmlPage);
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Looks quite good. I had little problems with HtmlPage understanding. What is the question? \$\endgroup\$ Mar 14, 2016 at 14:38
  • \$\begingroup\$ I'm looking for a code review in terms of maintainability and readability. Maybe the message in bounty about asking for an answer is misleading (but I can't edit it). I meant review, not answer ;) \$\endgroup\$ Mar 14, 2016 at 18:44

2 Answers 2

7
+50
\$\begingroup\$

Your outset is wrong. The articles you've linked to present a nice story, but what you're running into here is the issue with that ideology:

When your data structures AND your views are rigid, you can't change the system to do anything else.

You're now stuck!

Say I had to show a listing of Employees. I can't! There is no way to use the data stored in Employee to make a screen that contains a listing of employees. I also cannot make a REST API with this Employee object without some serious magic - I'd have to make a "View" which takes multiple Employees and ignores calls to "show" unless they're from an authorized source.

You say "Class Employee is not responsible for showing itself".

public final class Employee {

  //...
  public void showOn(View view) {
    view.addData("Name", name.toString())
        .addData("Salary", salary.toString())
        .show();
  }
}

What's the show() doing here? Why does the Employee decide when the view is ready? Maybe I have a page consisting of a Project and the Employees that work on it. I'd have to make custom subviews.

You're right in that the Employee does not decide HOW it is shown, but it does decide WHAT is shown and WHEN it is shown. Which is not a desired property.

That, or I'd have to alter Employee to add View logic to support multiple or partial employee views. Which is bad, because standards constantly change, and you don't want to alter all your model classes to be able to give a view that can be used for that standard (e.g. something that you can convert to JSON).

Additionally, by limiting yourself to those constraints, you can no longer leverage certain libraries! It's fun and all to harp on the gigantic projects and code cruft that comes with using big libraries like Hibernate and Spring and such, but if you need to build a REST API and a phone app, being able to have libraries handle your object conversion to json and back (and in case of android, maybe even SHARE the model objects as a library!)... you can't do it in your setup! There are no getters, objects are immutable, libraries don't support this.

Employee.View is oddly specific class; it doesn't make any mention of Employee:

  public interface View {
    View addData(String name, String value);
    void show();
  }

Let's say we had projects later - will there also be Project.View? What methods will it contain? The same? Mostly the same?

This leads to code duplication, which is really bad!

In your HTML page you use a table, and for that, String name, String value will do. But for non table situations, like a stackexchange profile, you have graphs for reputation, lists of answers and questions, it's partially sortable, some are numbers, others are text, some of the numbers are rounded (reputation is shown as ##.#k, rather than a 5 digit amount)...

All of that is either HARD or NOT POSSIBLE just because you're following the dogma of "You should never have Getters/Setters near your code.".

Such an idea is DANGEROUS in this case!

But you link to more than just 1 article of this guy, so maybe you really like what he says and I won't get through to you by just pointing at your code. Let's point at his:

Dog dog = new Dog();
dog.setBall(new Ball());

You could say this can be improved; Dog can have 0 to many objects to play with. Maybe it's carrying up to one object at a time. You could either use setCarriedObject (but that's secretly setBall), or, perhaps better, take(PlayObject). I call it PlayObject but basically anything that you could give to the dog to carry around would work. We're not using a very strict object hierarchy here.

Then, of course, if we're gonna play fetch, we'd have to retrieve the ball.

Dog dog = new Dog();
Ball ball = dog.getBall();

Because we've rephrased setBall to take (the dog takes the ball... because you've ordered him to), getBall is obviously going to become give.

But they're still getters and setters, we just call them differently now. They do the same.

The author comes to the same conclusion:

Dog dog = new Dog();
dog.take(new Ball());
Ball ball = dog.give();

So far, I get the point; getters and setters show that you might not have thought about the usecases and are treating your objects as dumb things, and maybe you want to emphasize that there's actual work being done.

But then the author says this:

Now, we're treating the dog as a real animal, who can take a ball from us and can give it back, when we ask. Worth mentioning is that the dog can't give NULL back. Dogs simply don't know what NULL is :) Object thinking immediately eliminates NULL references from your code.

How?!

If I do this:

Ball ballInMyHand = new Ball();
Dog dog = new Dog();
Ball otherBall = dog.give();
dog.take(ballInMyHand);

What's otherBall gonna be? The author says it's not going to be null. In that case, the options we've got left are these:

  • Null-representative value; the dog gives you "air" or a NonExistantBall
  • A new ball; the dog steals one from the neighborhood
  • The ball that was previously in my hand; the dog knows where all the balls are and can use reflection to obtain any ball
  • NoBallException; the dog goes mad and dies, possibly killing you (The program)
  • Infinite loop; the dog searches for a ball forever, and we're relying on the inherent multi-threaded way that the world works for a person to tell the dog to stop searching (but this doesn't happen, so you're stuck forever)
  • Program exits with fault code; the universe stops existing

And, save for the "air" option, they're all really bad choices! Exceptions for regular cases, infinite loops, program exits... what's so bad about null?

Back to Employees for a moment; say I want to generate a report that tells me all the costs I spend on Employees per month. That's salary, compensations, bonuses and all that. I sure hope there's an alternative data source out there, because the only way I'm gonna get to that data is by pretending I'm going to display it. I guess adding up all the salaries is gonna be done by casting String to int again.

It's also weird that I have to delete the old employee if I want to give them a raise.

You also have the issue that your views are stringly typed: if you wanted to prominently display Job title at the top, you'd have to filter via if(name.equals("Job title")). And if we ever change it to Job Title or Title or Role or Occupation then guess what, we're going to have to go back and change everything again! IDE's may or may not support this refactoring, so chances are you'd have to do all this by hand.


To summarize - what you've done presents multiple problems:

  • Views and Model are tightly coupled
  • New views require model alteration (violation of open-closed and SRP)
  • Less library support means more manual work
  • View class is likely to be duplicated
  • Model giving data rather than providing data means views are limited in what they can do

and I see no real benefits.


But lets say your boss says "I've read this article, and this stuff is amazing! You programmers should work to build our apps like this. Now, I've already discussed this with the managers and we're going with this, so if you don't like it, there's the door." There's no way you're going to convince your boss, he has decided that this way is the way. And you need your job, whilst quitting does sound nice, you need the money, and if you don't work you don't get paid.

What do you do?

In that case, take immutability up another level.

Instead of treating your objects like forms to be filled in and stored, like HtmlPage, create printers of a sort.

Employee goes in, EmployeeReport comes out. If you want a different report, either feed the Employee into a different ReportMaker, give the ReportMaker different settings, or pass more Employees into ReportMaker.

Pure functional programming like that exists, and whilst Java isn't the best language for it, Java 8 made things a lot better. You don't have to carry state around all the time. Once you have the report you need, you can drop all instances of objects and just request them new later.

\$\endgroup\$
6
  • \$\begingroup\$ "Views and Model are tightly coupled" - if View will use Employee's getters, they also are coupled; the only change is that View depends on Employee. Is this direction of dependency better even if violates Employee's encapsulation? "It's also weird that I have to delete the old employee if I want to give them a raise." - this is a good observation ;) But I'd go further: maybe Employee shouldn't keep its salary but, instead, a constant reference to object which keeps it: dataBase, accountingDepartment, lineManager or something like that? \$\endgroup\$ Mar 15, 2016 at 12:40
  • \$\begingroup\$ @PiotrAleksanderChmielowski the difference is that if I want multiple views, I need only make another view, whereas in your case I'd need to alter the employee to be able to give a different view. When you start getting several views (take a look around this site, there's main profile, chat profile, comment name, chat comments, chat UI, top bar UI, and they all contain either your name or your avatar), this gets messy. \$\endgroup\$
    – Pimgd
    Mar 15, 2016 at 12:46
  • \$\begingroup\$ OK, I got it. So, let's modify View. Now it does not have addData member but knows something about Employee and offer methods: addName(String nm), addAvatar(Avatar avtr) or simply show(Name nm, Avatar avtr). Then it has the data provided by Employee without asking for it and decide what to show and how. \$\endgroup\$ Mar 15, 2016 at 13:00
  • \$\begingroup\$ Why am I stubborn with my view, that View can not ask Employee for data? Because IMO if Employee provides data to View himself, it's clear why is he doing it. Let's take a function from my previous comment: view.show(this.name, this.avatar); - it is clear: Employee provided name and avatar to view because View has a contract: the function show which requires them as parameters. If we use getters, by looking at Employee it is hard to find their purpose. \$\endgroup\$ Mar 15, 2016 at 13:03
  • 1
    \$\begingroup\$ I'm still not convinced to each of your point. However, you show very precisely a lot of weak points in my ideas and I gave me a very good food for thought. Therefore the bounty is yours :) \$\endgroup\$ Mar 16, 2016 at 12:05
4
\$\begingroup\$

In addition to Pimgd's excellent review:

Use StringBuilder to build strings

String concatenation by operator + causes many object allocations unnecessarily, you can use a string builder like this to avoid having to return a new HtmlPage every time too:

final class HtmlPage implements Employee.View {
  private final StringBuilder content = new StringBuilder();

  public HtmlPage() {
  }

  public HtmlPage(String str) {
    content.append(str);
  }

  @Override
  public Employee.View addData(String name, String value) {
    content.append("\t<tr><td>")
           .append(name)
           .append("</td><td>")
           .append(value)
           .append("</td></tr>\n");
    return this;
  }

  @Override
  public void show() {
    System.out.println("<table>");
    System.out.println(content.toString());
    System.out.println("</table>");
  }
}

Accessors (Setters & Getters)

Although getters and setters can be a code smell avoiding them in this convoluted way that you're trying to is the wrong way to go.

Look at the description of Employee, it has no functionality, it is not an object in an OO sense; it is simply a data structure, an imaginary row in a database table somewhere. Because of this, it is natural for it to have accessors for its members.

It is perfectly acceptable for your data model/class to have accessors.

If you had an object with behavior, for example:

public interface Worker{
    void performWork();
    void takeBreak();
}

then the picture changes a bit. The Worker class is an active class that performs some function in the program, other than just holding a bunch of data. Note that this type of object is rarely shown in the UI, although its function may be triggered from an action in the UI. If this interface would contain data it would have methods called for example set/getName(). Which in an interface class would look odd as it prescribes the interface to have certain data members in which case it might just as well be an abstract class.

I find through experience that separating behavior and aggregate calculations from your data model will avoid a lot of troubles such as classes growing too large or having mixed concerns. And it helps in keeping classes small and with good cohesion. Especially when you have anything but a trivial data model.

In closing I want to say that accessors are not automatically bad by them selves, they only become a problem when they are used blindly without first considering if the functionality they are being used by should be a part of the class itself. When used to access attributes of your data model for display or use by the behavior, they are perfectly fine.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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