trjhtr

Follow up question to:
Utilizing OOP when creating a web browser

After I received help on the question above, I've created 3 classes for my Browser. Browser is my "View"-class that handles the GUI, Model is self-explaining and History contains the lists and their functionality.

First of all, is this an OK solution from a MVC-perspective? And also, my History class uses static counters because that was the best solution I could think of considering I want them to be the same no matter where they're used, but is there a better way to use counters without having them static? I've been told that using static should be avoided while in the noob stages of programming.

Browser:

import java.awt.BorderLayout;
import java.awt.event.*;
import javax.swing.*;
import javax.swing.event.*;

public class Browser

 public JTextField addressBar;
 public JEditorPane display;
 private JButton button;
 private JPanel panel;
 private String URL = new String();
 private JButton backward;
 private JButton forward;
 private JFrame frame;
 private History history;

 public Browser(Model controller, History history) 
 this.history = history;
 frame = new JFrame("My Browser");
 panel = new JPanel();
 addressBar = new JTextField("Enter a URL");
 addressBar.addActionListener(new ActionListener() 
 public void actionPerformed(ActionEvent e) 
 URL = e.getActionCommand().toString();
 controller.newURL(URL);
 
 );
 display = new JEditorPane();
 display.setEditable(false);
 display.addHyperlinkListener(new HyperlinkListener() 
 public void hyperlinkUpdate(HyperlinkEvent e) 
 if (e.getEventType() == HyperlinkEvent.EventType.ACTIVATED) 
 URL = e.getURL().toString();
 controller.newURL(URL);
 
 
 );
 addButton("Close").addActionListener(new ActionListener() 
 public void actionPerformed(ActionEvent e) 
 if (e.getSource() != null) 
 controller.closeAction();
 
 
 );

 forward = addButton("Forward");
 forward.setEnabled(false);
 forward.addActionListener(new ActionListener() 
 public void actionPerformed(ActionEvent e) 
 if (e.getSource() != null) 
 controller.forwardAction();
 
 
 );

 backward = addButton("Back");
 backward.setEnabled(false);
 backward.addActionListener(new ActionListener() 
 public void actionPerformed(ActionEvent e) 
 if (e.getSource() != null) 
 controller.backwardAction();
 
 
 );

 addButton("History").addActionListener(new ActionListener() 
 public void actionPerformed(ActionEvent e) 
 if (e.getSource() != null) 
 controller.historyAction();
 
 
 );
 setframe();
 

 private void setframe() 
 frame.add(addressBar, BorderLayout.NORTH);
 frame.add(panel, BorderLayout.SOUTH);
 frame.add(new JScrollPane(display), BorderLayout.CENTER);
 frame.setSize(400, 400);
 frame.setVisible(true);
 

 private JButton addButton(String name) 
 button = new JButton(name);
 panel.add(button);
 return button;
 

 public void initBackward() 
 if (History.i > 0) 
 backward.setEnabled(true);
 else 
 backward.setEnabled(false);
 
 

 public void initForward() 
 if (History.i < history.list.size() - 1) 
 forward.setEnabled(true);
 else 
 forward.setEnabled(false);
 
 

Model:

import javax.swing.JEditorPane;
import javax.swing.JOptionPane;
import javax.swing.event.HyperlinkEvent;
import javax.swing.event.HyperlinkListener;

public class Model
 private Browser browser;
 private History history;

 public Model() 
 history = new History();
 browser = new Browser(this, history);
 

 public void loadURL(String URL) 
 try 
 browser.display.setPage(URL);
 browser.addressBar.setText(URL);
 catch (Exception e) 
 history.removefromList(history.list);
 history.removefromList(history.previousPages);
 JOptionPane.showMessageDialog(null, "fel länk");
 
 

 public void closeAction() 
 System.exit(0);
 

 public void forwardAction() 
 History.i++;
 browser.initBackward();
 browser.initForward();
 loadURL(history.list.get(History.i));
 

 public void backwardAction() 
 History.i--;
 browser.initForward();
 browser.initBackward();
 loadURL(history.list.get(History.i));
 

 public void historyAction() 
 String html = new String();
 for (String link : history.previousPages) 
 html = html + "<a href="" + link + "">" + link + "</a>n";
 
 html = "<html><body" + html + "</body></html>";
 JEditorPane ep = new JEditorPane("text/html", html);
 ep.addHyperlinkListener(new HyperlinkListener() 
 public void hyperlinkUpdate(HyperlinkEvent e) 
 if (e.getEventType().equals(HyperlinkEvent.EventType.ACTIVATED)) 
 loadURL(e.getURL().toString());
 
 
 );
 ep.setEditable(false);
 JOptionPane.showMessageDialog(null, ep);
 


 public void newURL(String URL) 
 History.i++;
 History.j++;
 history.checklist();
 loadURL(URL);
 history.addtoList(history.previousPages, URL);
 history.addtoList(history.list, URL);
 browser.initBackward();
 browser.initForward();
 

History:

import java.util.ArrayList;
import java.util.List;

public class History 
 public static int i = -1;
 public static int j = -1;
 public List<String> list = new ArrayList<>();
 public List<String> previousPages = new ArrayList<>();

 public void checklist() 
 if (i != j) 
 list.subList(i, list.size()).clear();
 j = i;
 
 

 public void addtoList(List<String> list, String URL) 
 list.add(URL);
 

 public void removefromList(List<String> list) 
 list.remove(list.size()-1);
 

MyFrame:

public class MyFrame 

 public static void main(String args) 
 new Model();
 

First I must apologise for calling it MVC in my answer to your original question. After reading gervais.b's answer I realised that what I call "MVC" isn't the usual way to implement it. After some searching I found out that what I tried to explain is better known as MVA (model view adapter).

The main difference is that I take a "view" as the GUI (in your case the Browser class). I have the History as the "model" (which in MVA is NOT coupled directly to the View). And finally I have some binding class that provides the coupling between a View and the Model (which I "wrongly" called the Controller and from which you probably got confused). So let's fix it while providing more tips to improve your design.

First step: renaming the Model class to something different. My first thought would be something like BrowserHistoryBinding but this name is open for suggestions.

To be even more obvious let's also rename Browser to BrowserGUI, which again is also open for suggestions (even keeping it Browser could be OK, you can always refactor class names later on if you can think of a better name).

Next let's fix this:

I've been told that using static should be avoided while in the noob stages of programming.

Although static fields have their uses (strongly discouraged though) the i and j variables in the History class should not be static. Just remove the static modifier. (Note this breaks all references but we'll fix those later). A good example of static fields is when they are constants, these are also final and follow different naming conventions. (See the BLANK_PAGE constant in the example bellow).

Since we're cleaning up the History class we might as well look at what state it should and should not have.

The variable j represents the number of items in the history. We don't need this variable since list.size() gives us this information already. So j can be removed completely.

The variable previousPages isn't used. This can also be removed completely. On the other hand, the name previousPages actually fits the intention of the variable list a lot better. So let's rename list to previousPages.

Lastly, the variable i could also use a more meaningful name. From what I can tell it represents the index of the current page so let's call it currentPageIndex instead.

Now that we reduced the class to it's essential state, let's also look at what we need to provide to get information about this state and add new pages to the history.

public class History 
 public static final String BLANK_PAGE = "about:blank";
 private List<String> previousPages = new ArrayList<String>();

 private int currentPageIndex = 0;

 /**
 * Removes any existing pages after the current active page and than adds the newUrl to the history.
 */
 public void addUrl(String newUrl) 
 removeNextPages();
 previousPages.add(newUrl);
 currentPageIndex++;
 

 private void removeNextPages() 
 previousPages.subList(currentPageIndex, previousPages.size()).clear();
 

 public boolean hasPreviousPage() 
 return currentPageIndex > 0;
 

 public boolean hasNextPage() 
 return currentPageIndex < previousPages.size() - 1;
 

 /**
 * Goes back 1 page in the history and returns that page.
 * If no previous page exists return "about:blank" instead to default to a blank page.
 */
 public String goBack() 
 if (!hasPreviousPage()) 
 return BLANK_PAGE;
 

 currentPageIndex--;
 return previousPages.get(currentPageIndex);
 


 /**
 * Goes forward 1 page in the history and returns that page.
 * If no next page exists return "about:blank" instead to default to a blank page.
 */
 public String goForward() 
 if (!hasNextPage()) 
 return BLANK_PAGE;
 

 currentPageIndex++;
 return previousPages.get(currentPageIndex);
 

Some things to notice here.

No other class has access to the state of the History except through the public methods. Other classes don't even know that the history is stored in a list and that it keeps track of the current page using an int. If we ever want to change the implementation to something else only this class needs to change.

removeNextPages is private since no other class needs to call this method. It is called from inside the addUrl method since that is the only time we ever want to remove any pages from our list.

goBack and goForward check if the input is valid. I have chosen to just keep the internal state consistent and return a blank page. An alternative solution could be to throw an exception instead. I have added the required method comments to tell the person using these methods what he can expect.

Now that we cleaned up the History class let's take a look at the Browser class.

This one already looks pretty good actually. Except for 2 "major" issues and some minor nitpicks.

Problem 1: There are public fields. Just like in the History class, we don't want any other class to know about how we store the state of this class. Any public field means we can't change it afterwards without also touching other classes. Let's just make these private for now and fix the remaining BrowserHistoryBinding class when we're done with this one.

Problem 2: BrowserGUI should have no references to the History class. Just remove the History parameter from the constructor (it isn't used anyway). And replace the initBackward and initForward methods with the following 2:

public void setBackwardEnabled(boolean enabled) 
 backward.setEnabled(enabled);


public void setForwardEnabled(boolean enabled) 
 forward.setEnabled(true);

Note that checking the logic whether or not these 2 buttons should be enabled is not the responsibility of the browser class itself. Someone else will check the logic and then tell the Browser to enable or disable these buttons.

For the minor nitpicks:
Notice how I renamed the initX methods to setXEnabled. This was probably also my fault for not being clear about the naming convention on my other answer. initX is reserved for helper methods used during construction (or delayed initialisation) of the class. Here we're set-ing new values during normal use of the class. So set-ers are the default names for these methods.

For that same reason your setFrame method should be renamed to initFrame since this method is used to initialise the frame on construction.

You shouldn't have a field button. This should just be defined locally inside the method instead:

private JButton addButton(String name) 
 JButton button = new JButton(name);
 panel.add(button);
 return button;

You also don't need the URL field. Just define it as String at the 2 places it's used:

 addressBar.addActionListener(new ActionListener() 
 @Override
 public void actionPerformed(ActionEvent e) 
 String url = e.getActionCommand();
 controller.newURL(url);
 
 );

 display.addHyperlinkListener(new HyperlinkListener() 
 @Override
 public void hyperlinkUpdate(HyperlinkEvent e) 
 if (Objects.equals(e.getEventType(), HyperlinkEvent.EventType.ACTIVATED)) 
 String url = e.getURL().toString();
 controller.newURL(url);
 
 
 );

Notice also that I let my IDE change the == to Objects.equals( ... ) instead. String equality shouldn't be checked with ==.

But enough with the nitpicking. These minor details should be pointed out to you by using a decent IDE. For example, I use IntelliJ. Another popular one is Eclipse, both free to use, but there are also others so your choice which one to use.

Finally let's look at how to fix the binding class. Since we made the state of the other 2 classes completely private we broke most of this class.

Let's start at the top with the loadURL method. This method is only ever used inside this class, so we can make it private. Other classes probably shouldn't call this one directly anyway. We also encounter our first problem. Namely that we didn't provide a way to make the BrowserGUI actually show a certain URL. So let's start by adding that method to the ´BrowserGUI` class:

public void showURL(String url) throws IOException 
 display.setPage(url);
 addressBar.setText(url);

Notice how the display.setPage(url) might throw an exception but that we're not interested in handling that exception here. Instead we just let the entire method throw in to the caller and have them handle it. With this method added we can continue in the binding class.

public void loadURL(String URL) 
 try 
 browser.display.setPage(URL);
 browser.addressBar.setText(URL);
 catch (Exception e) 
 //history.removefromList(history.list);
 //history.removefromList(history.previousPages);
 JOptionPane.showMessageDialog(null, "fel länk");
 
 

For simplicity's sake I just commented out the history.removefromList lines. You could provide a method in the history class to clear the history and call that one history.clearHistory() to achieve the same result as you got now.

On to the next broken method:

 public void forwardAction() 
 String newUrl = history.goForward();
 browser.setForwardEnabled(history.hasNextPage());
 browser.setBackwardEnabled(history.hasPreviousPage());
 loadURL(newUrl);
 

Notice how I'm now telling the history to goForward instead of accessing it's state and updating that directly. The History class itself will handle everything to keep it's state consistent and will return the next url in the history. Our binding class doesn't need to know how it does that.

Same thing for enabling the forward and backward buttons on the browser. We're now just telling the browser to enable it's buttons based on the given information. At the same time we're retrieving that information from the history because the binding class doesn't have to know what it means to "have a previous page" or a "next page". That's something only the History class should know since it's information about our Model.

Same reasoning for the next method:

public void backwardAction() 
 String newUrl = history.goBack();
 browser.setForwardEnabled(history.hasNextPage());
 browser.setBackwardEnabled(history.hasPreviousPage());
 loadURL(newUrl);

And now that I think about it, every time we change the URL we also want to update the forward and backward buttons. So why not put those inside the loadURL method instead?

public void loadURL(String url) 
 try 
 browser.showURL(url);
 catch (Exception e) 
 JOptionPane.showMessageDialog(null, "fel länk");
 

 browser.setForwardEnabled(history.hasNextPage());
 browser.setBackwardEnabled(history.hasPreviousPage());


public void forwardAction() 
 loadURL(history.goForward());


public void backwardAction() 
 loadURL(history.goBack());

And while we're at it let's also fix the newURL method with the same reasoning:

public void newURL(String url) 
 history.addUrl(url);
 loadURL(url);

Doesn't that look a lot simpler than what it used to be?

Finally there's the historyAction method where we get into a bit of trouble. Here we actually want to use the list of pages in the History but at the same time I told you that we don't want to make the state of any class public. So let's solve this by providing a getter in the History class to retrieve the entire list of pages:

public List<String> getAllPages()
 return previousPages;

This may look silly since we still just give access to our internal list right? But there's a good reason for it. Previously I kept saying that whenever we want to change the implementation it should only change that specific class right? Does that still hold? Well, let's say for example that instead of 1 list with the full history we would split it up into 2 lists. The previousPages that now only contains the pages before the current page. And another list nextPages that contains all the pages after the current page. We then change the implementation of this getter to:

public List<String> getAllPages()
 List<String> result = new ArrayList<String>(previousPages);
 result.addAll(nextPages);
 return result;

Notice how the signature of this method did not change. Meaning any class that already used it before can just use the updated version without ever knowing that the internal representation has changed.

Now to finish what we started here's the fixed implementation for your historyAction method.

public void historyAction() 
 String html = "";
 for (String link : history.getAllePages()) 
 html = html + "<a href="" + link + "">" + link + "</a>n";
 
 html = "<html><body" + html + "</body></html>";
 JEditorPane ep = new JEditorPane("text/html", html);
 ep.addHyperlinkListener(new HyperlinkListener() 
 @Override
 public void hyperlinkUpdate(HyperlinkEvent e) 
 if (e.getEventType().equals(HyperlinkEvent.EventType.ACTIVATED)) 
 loadURL(e.getURL().toString());
 
 
 );
 ep.setEditable(false);
 JOptionPane.showMessageDialog(null, ep);

And everything should work again. Note however that it would actually be better to let the view handle showing the user this list of previous pages. But that's something for a next update.

I hope this rather long post provided you with a better insight in how I would implement this browser application. As gervais.b has shown this is not the only possible way. There's advantages and dissadvantages as always. So it's worth studying other ways as well and try to find out which implementation is the best fit for your program. At least the general consensus is that it's a good idea to try to separate GUI code from business logic.

is this an OK solution from a MVC-perspective?

No it is not. Your view can be the Browser but your Model is the model and the controller (you name it controller in your browser by the way). And History is a model.

The Model must be used to contains your browser state (url, content, history).

class BrowserModel extends Observable 

 void goTo(URL newUrl) 
 history.add(newUrl); // Hide your i++, j++ and other logic
 content = read(newUrl);
 onChange();
 

The view (Browser) must be able to listen to the model and update when it changes.

void onUrlChanged(URL newUrl) 
 addressBar.setText(newUrl);


void onContentChanged(byte content) 
 display.setContent(new String(content, ..));

The missing controller must be responsible to react to user events. It can either listen to events from Browser components or use a more generic mechanism (in case you want to change your view technology). It react to those events by changing the view state and/or updating the model.

view.addressBar.addActionListener(new ActionListener() 
 public void actionPerformed(ActionEvent e) 
 URL target = e.getActionCommand().toString();
 view.setLoading();
 model.goTo(target);
 
 );

Those simple changes will move you to a better MVC model where the controller react to user input to change the model and where the view refresh itself when the model changes.

When you are there, you can start to encapsulate your components and use meaningful names. Especially for History where the logic to add a new url must be hidden within the class itself.