1
\$\begingroup\$

I am creating project named MEdit (link). It's rather advanced text editor, which features:

  1. Syntax highlighting for around 91 languages.

  2. Is pretty well commented

  3. Is written in pure Java

  4. Possibility of scripting using B++ ("extension" for Javascript)

  5. Text operations (lowercase, upercase, randomcase)

  6. Regex search and replace.

  7. Nice UI, 9 color schemes

  8. Jump to line feature, editor never hangs on loading files even as big as 5GB (proven)

  9. Cyclical GC, if memory ussage extends 300mb.

  10. Small memory footprint.

  11. Possibility to link your own compiler to use with MEdit.

  12. Possiblity of creating own autocompletion for desired language.

Actually there are only such features, but I'm planning much more. I'd want to hear, what's done good, and what's done worse in my project. I'm not so advanced with Java, so I'd like to see review of my code. Only thing that i didn't do was big time testing - there are just some simple tests and wiki pages about creating own autocomplete style, and creating own tool and script. It's not recommended to download binaries directly, as they are rarely updated.

Small parts of source code:

MainFrame.java

package medit;

import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.Toolkit;
import java.io.File;
import java.io.IOException;

import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JPanel;
import javax.swing.JRadioButtonMenuItem;
import javax.swing.JToolBar;
import javax.swing.WindowConstants;
import javax.swing.border.EmptyBorder;

import org.fife.ui.rsyntaxtextarea.RSyntaxTextArea;
import org.fife.ui.rsyntaxtextarea.SyntaxConstants;
import org.fife.ui.rsyntaxtextarea.Theme;
import org.fife.ui.rtextarea.RTextScrollPane;

import medit.ActionManagers.AboutActionManager;
import medit.ActionManagers.BottombarActionManager;
import medit.ActionManagers.CodeCompletionActionManager;
import medit.ActionManagers.EditActionManager;
import medit.ActionManagers.FileActionManager;
import medit.ActionManagers.LanguageActionManager;
import medit.ActionManagers.ScriptsActionManager;
import medit.ActionManagers.TextOPActionManager;
import medit.ActionManagers.ThemesActionManager;
import medit.ActionManagers.TimerTaskActionManager;
import medit.ActionManagers.ToolActionManager;
import medit.ActionManagers.WindowActionManager;

/**
 * Main frame for MEdit project. That's where the whole magic is done. It was
 * split to many files, which are located in ActionManagers.
 *
 * @author Krzysztof Szewczyk
 */

public class MainFrame extends JFrame {

    /**
     * Many public variables, that were privatized before. They are public, because
     * our MainFrame is not standalone class now and it references many
     * ActionManagers.
     */

    public static int instances = 1;
    public static final long serialVersionUID = 1L;
    public JPanel contentPane;
    public File currentFile = null;
    public MainFrame instance;
    public final JLabel lblReady = new JLabel(
            "Ready | Length: 0 | Filename: \"Unnamed\" | Maximum size: 0KB | INS | LCK | SCR");
    public final RSyntaxTextArea textPane = new RSyntaxTextArea();

    /**
     * Create the frame.
     */
    public MainFrame() {

        /**
         * Frame setup
         */
        this.instance = this;
        this.setIconImage(Toolkit.getDefaultToolkit()
                .getImage(MainFrame.class.getResource("/medit/assets/apps/accessories-text-editor.png")));
        this.setTitle("MEdit");
        this.setDefaultCloseOperation(WindowConstants.DISPOSE_ON_CLOSE);
        this.setBounds(100, 100, 700, 500);
        this.setMinimumSize(new Dimension(700, 500));
        this.contentPane = new JPanel();
        this.contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
        this.setContentPane(this.contentPane);
        this.contentPane.setLayout(new BorderLayout(0, 0));

        /**
         * Menu bar Setup
         */
        final JMenuBar menuBar = new JMenuBar();
        this.setJMenuBar(menuBar);

        /**
         * Menus setup
         */
        final JMenu mnFile = new JMenu("File");
        menuBar.add(mnFile);
        final JMenu mnEdit = new JMenu("Edit");
        menuBar.add(mnEdit);
        final JMenu mnLanguage = new JMenu("Language");
        menuBar.add(mnLanguage);
        final JMenu mnSyntaxHighlighting = new JMenu("Syntax Highlighting");
        menuBar.add(mnSyntaxHighlighting);
        final JMenu mnThemes = new JMenu("Themes");
        menuBar.add(mnThemes);
        final JMenu mnTools = new JMenu("Tools");
        menuBar.add(mnTools);
        final JMenu mnScripts = new JMenu("Scripts");
        menuBar.add(mnScripts);
        final JMenu mnAbout = new JMenu("About");
        menuBar.add(mnAbout);
        final JMenu mnTextOperations = new JMenu("Text Operations");
        menuBar.add(mnTextOperations);

        /**
         * Menu action managers setup.
         */

        final WindowActionManager wam = new WindowActionManager(this);
        wam.Closing();

        final FileActionManager fam = new FileActionManager(this);
        fam.New(mnFile);
        fam.Open(mnFile);
        fam.Save(mnFile);
        fam.SaveAs(mnFile);
        fam.Print(mnFile);
        fam.Separator(mnFile);
        fam.ReloadFromDisk(mnFile);
        fam.OpenDir(mnFile);
        fam.RemoveFromDisk(mnFile);
        fam.Separator(mnFile);
        fam.Exit(mnFile);

        final EditActionManager eam = new EditActionManager(this);
        eam.Cut(mnEdit);
        eam.Copy(mnEdit);
        eam.Paste(mnEdit);
        eam.Delete(mnEdit);
        eam.Separator(mnEdit);
        eam.Undo(mnEdit);
        eam.Redo(mnEdit);
        eam.Separator(mnEdit);
        eam.Search(mnEdit);

        final TextOPActionManager topam = new TextOPActionManager(this);
        topam.SetupTextOP(mnTextOperations);

        final AboutActionManager aam = new AboutActionManager();
        aam.About(mnAbout);

        final CodeCompletionActionManager ccam = new CodeCompletionActionManager(this);
        ccam.SetUpCodeCompletion(SyntaxConstants.SYNTAX_STYLE_NONE);

        final LanguageActionManager lam = new LanguageActionManager(this);
        lam.SetUp(mnSyntaxHighlighting, ccam);

        final ThemesActionManager tam = new ThemesActionManager(this);
        tam.RegisterThemes(mnThemes);

        final TimerTaskActionManager ttam = new TimerTaskActionManager(this);
        ttam.SetUpTimers();

        final BottombarActionManager bbam = new BottombarActionManager(this);
        bbam.SetUpBottombar();

        final ToolActionManager toolam = new ToolActionManager(this);
        toolam.SetupTools(mnTools);

        final ScriptsActionManager sam = new ScriptsActionManager(this);
        sam.SetupScripts(mnScripts);

        /**
         * Language submenu setup
         */
        final JRadioButtonMenuItem rdbtnmntmEnglish = new JRadioButtonMenuItem("English");
        rdbtnmntmEnglish.setSelected(true);
        mnLanguage.add(rdbtnmntmEnglish);

        /**
         * Toolbar setup.
         */
        final JToolBar toolBar = new JToolBar();
        toolBar.setFloatable(false);
        this.contentPane.add(toolBar, BorderLayout.NORTH);

        fam.New(toolBar);
        fam.Open(toolBar);
        fam.Save(toolBar);
        fam.Exit(toolBar);

        eam.Cut(toolBar);
        eam.Copy(toolBar);
        eam.Paste(toolBar);
        eam.Delete(toolBar);
        eam.Undo(toolBar);
        eam.Redo(toolBar);

        /**
         * Editor setup
         */
        final RTextScrollPane scrollPane = new RTextScrollPane();
        this.contentPane.add(scrollPane, BorderLayout.CENTER);

        this.textPane.setFont(new Font("Monospaced", Font.PLAIN, 13));
        scrollPane.setViewportView(this.textPane);

        this.textPane.clearParsers();
        this.textPane.setParserDelay(1);
        this.textPane.setAnimateBracketMatching(true);
        this.textPane.setAutoIndentEnabled(true);
        this.textPane.setAntiAliasingEnabled(true);
        this.textPane.setBracketMatchingEnabled(true);
        this.textPane.setCloseCurlyBraces(true);
        this.textPane.setCloseMarkupTags(true);
        this.textPane.setCodeFoldingEnabled(true);
        this.textPane.setHyperlinkForeground(Color.pink);
        this.textPane.setHyperlinksEnabled(true);
        this.textPane.setPaintMatchedBracketPair(true);
        this.textPane.setPaintTabLines(true);
        scrollPane.setIconRowHeaderEnabled(true);
        scrollPane.setLineNumbersEnabled(true);
        try {
            final Theme theme = Theme
                    .load(this.getClass().getResourceAsStream("/org/fife/ui/rsyntaxtextarea/themes/default.xml"));
            theme.apply(this.textPane);
        } catch (final IOException ioe) { // Never happens
            final Crash dialog = new Crash(ioe);
            dialog.setDefaultCloseOperation(WindowConstants.DISPOSE_ON_CLOSE);
            dialog.setVisible(true);
        }
        scrollPane.setLineNumbersEnabled(true);
        scrollPane.setFoldIndicatorEnabled(true);

    }

}

Crash.java:

package medit;

import java.awt.BorderLayout;
import java.awt.FlowLayout;
import java.awt.Font;
import java.awt.Toolkit;
import java.io.PrintWriter;
import java.io.StringWriter;

import javax.swing.JButton;
import javax.swing.JDialog;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.SwingConstants;
import javax.swing.border.EmptyBorder;

/**
 * Crash dialog that appears after some exception is thrown.
 *
 * @author Krzysztof Szewczyk
 *
 */

public class Crash extends JDialog {

    private static final long serialVersionUID = 1L;
    private final JPanel contentPanel = new JPanel();

    /**
     * Create the dialog.
     */
    public Crash(final Exception E1) {
        this.setIconImage(Toolkit.getDefaultToolkit()
                .getImage(Crash.class.getResource("/medit/assets/actions/process-stop.png")));
        this.setTitle("MEdit");
        this.setBounds(100, 100, 450, 300);
        this.getContentPane().setLayout(new BorderLayout());
        this.contentPanel.setBorder(new EmptyBorder(5, 5, 5, 5));
        this.getContentPane().add(this.contentPanel, BorderLayout.CENTER);
        this.contentPanel.setLayout(new BorderLayout(0, 0));
        {
            final JLabel lblAnErrorOccured = new JLabel("An error occured.");
            lblAnErrorOccured.setHorizontalAlignment(SwingConstants.CENTER);
            this.contentPanel.add(lblAnErrorOccured, BorderLayout.NORTH);
        }
        {
            final JScrollPane scrollPane = new JScrollPane();
            this.contentPanel.add(scrollPane, BorderLayout.CENTER);
            {
                final JTextArea txtr = new JTextArea();
                final StringWriter sw = new StringWriter();
                final PrintWriter pw = new PrintWriter(sw);
                E1.printStackTrace(pw);
                final String sStackTrace = sw.toString();
                txtr.setText(sStackTrace);
                txtr.setFont(new Font("Monospaced", Font.PLAIN, 13));
                scrollPane.setViewportView(txtr);
            }
        }
        {
            final JPanel buttonPane = new JPanel();
            buttonPane.setLayout(new FlowLayout(FlowLayout.RIGHT));
            this.getContentPane().add(buttonPane, BorderLayout.SOUTH);
            {
                final JButton okButton = new JButton("Exit");
                okButton.addActionListener(e -> System.exit(0));
                buttonPane.add(okButton);
                this.getRootPane().setDefaultButton(okButton);
            }
            {
                final JButton cancelButton = new JButton("Continue");
                cancelButton.addActionListener(e -> Crash.this.dispose());
                buttonPane.add(cancelButton);
            }
        }
    }

}

NSSLoader.java (Script loader):

package medit.NSS;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import javax.swing.WindowConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;

import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

import medit.Crash;

/**
 * This class is loading scripts for new script system.
 * 
 * @author Krzysztof Szewczyk
 *
 */

public class NSSLoader {

    private final List<NSSEntry> tools = new ArrayList<>();

    /**
     * This function is loading every scripts from selected file.
     * 
     * @param string
     * @return
     * @throws ParserConfigurationException
     * @throws SAXException
     * @throws IOException
     */

    public List<NSSEntry> loadAll(final String string) throws ParserConfigurationException, SAXException, IOException {
        if (!new File(string).exists())
            return null;
        final File inputFile = new File(string);
        final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
        final DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
        final Document doc = dBuilder.parse(inputFile);
        doc.getDocumentElement().normalize();
        if (doc.getDocumentElement().getNodeName() != "medit") {
            final Crash dialog = new Crash(
                    new Exception("Parent element in script config file has to be equal to \"medit\"!"));
            dialog.setDefaultCloseOperation(WindowConstants.DISPOSE_ON_CLOSE);
            dialog.setVisible(true);
        }
        final NodeList nList = doc.getElementsByTagName("script");
        for (int temp = 0; temp < nList.getLength(); temp++) {
            final Node nNode = nList.item(temp);
            if (nNode.getNodeType() == Node.ELEMENT_NODE) {
                final Element eElement = (Element) nNode;
                final String name = eElement.getElementsByTagName("name").item(0).getTextContent();
                final String script = eElement.getElementsByTagName("scriptfile").item(0).getTextContent();
                this.tools.add(new NSSEntry(name, script) {
                    @Override
                    public String getCodeFN() {
                        return this.codefn;
                    }

                    @Override
                    public String getName() {
                        return this.name;
                    }
                });
            }
        }
        return this.tools;
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ Third party links are allowed, but you should include the code in the question. \$\endgroup\$ – Ludisposed Feb 3 '18 at 18:24
  • \$\begingroup\$ Whole code is 47kB, shall i post everything here? \$\endgroup\$ – Krzysztof Szewczyk Feb 3 '18 at 18:25
  • 1
    \$\begingroup\$ You should post a couple of classes (less than 10 and less than 500 LoC over all) you think needing some improvement. \$\endgroup\$ – Timothy Truckle Feb 3 '18 at 19:49
2
\$\begingroup\$

Let me start with general comments about your featureset and then I'll review the code:

Swing is EOL

Swing has been deprecated. Like... two years ago? Everything points to using JavaFX and I'm personally inclined to agree, even though the paradigms used by these two UI frameworks are vastly different...

JavaFX just has the more modern paradigm, makes better use of Language features that have been there for years (Generics for one). Additionally JavaFX simplifies some of the features you describe to "switch this resource for that resource"...

I strongly recommend checking out JavaFX.

Preferences vs. Settings.

I want to preemptively note that making a text-editor highly extensible comes with a host of issues, the least of which is that users will be overwhelmed with the sheer amount of features.

9 Color Schemes seems like a lot. Especially if you could just allow an arbitrary number of color schemes, and just allow customization...

"Nice UI" is highly subjective.

In closing I want to mention an article I recently read: Preferences Considered Harmful

On to the code

I'm going to go through the code from top to bottom, just commenting what comes to mind, pretty unfiltered.

package medit;

Package names should be domains in reverse order. I'd have expected com, net, org or anything like that in the first place.

import java.awt.[...];

AWT is something you don't really want to be in contact with directly. It's sure problematic that Java somewhat locked itself into the early nineties of UI-Design, by making raw awt a public API (and guaranteeing it's backwards-compatibility).

The other problem with awt is that it doesn't help you abstract UI concerns well. It's just onerous to write awt code, not very different from calling OS-routines directly, apart from being on the Java level instead of syscalls.
This directly implies that all your UI handling code will be very low-level, which makes it needlessly hard to understand.

import java.io.File;

Java has introduced an improved way of handling I/O. The java.nio package has replaced java.io in all but deprecation warnings.

A problem of Swing is that it really only uses java.io API, which makes it more difficult than necessary to correctly and cleanly handle I/O problems.

import medit.ActionManagers.[...];

I'm somewhat losing my shit here. This host of import statements implies that you're heavily overusing static contexts, as well as Managers.

This implies that you have difficulty explicitly structuring the code as Object Oriented. I'm glad you're not using explicit Singletons (at least here), but this is only marginally better.

Assuming the size of the program that your advertised feature-set implies, you should really set up dependency injection. That reduces Manager-God-Classes that are responsible for everything ever to do with $Component.

I concede that that may increase memory-footprint a little, as well as GC-times, but that's not something I'd worry about unless it's proven to be a problem ...

/**
 * Main frame for MEdit project. That's where the whole magic is done. It was
 * split to many files, which are located in ActionManagers.
 *
 * @author Krzysztof Szewczyk
 */

This javadoc comment nicely illustrates the points I was trying to make above. Especially the part about splitting this into many files really drives my point home. This is a god-class that does everything ever.

To get "proper Object Oriented" code, you need to embrace task delegation.

public class MainFrame extends JFrame {

Composition Over Inheritance is calling for one. I may have one or the other rant about this in older answers of mine. Generally I recommend reading through the questions here on the site to get a better feeling for how you can write Swing-Code that doesn't use this.

I'm glad you did not use the abomination that's implements MouseListener, ActionListener, [...].

/**
 * Many public variables, that were privatized before. They are public, because
 * our MainFrame is not standalone class now and it references many
 * ActionManagers.
 */

This further drives the point about being a god-class. You're extremely tightly coupling the MainFrame to all the components (and reverse). That makes the code resistant to small and contained changes.

Instead changing something usually will result in needing to touch a great number of files. You want to avoid that, so that instead of chasing changes across the whole codebase, you can confidently change small units of code. That usually also makes it a lot easier to reason about changes, since you don't need to consider the implications of a change on the whole program (mutliple thousand lines of code), but can reason about them in the context of your class (or worst case package).

public static int instances = 1;

A type generally doesn't need to know how many instances of it exist. This is very unusual and weird. Why do you need it?

 public static final long serialVersionUID = 1L;

This doesn't implement Serializable. Also this is the default UID. Which is not a UID at all and defeats the whole point of one.

Smells like Cargo-Culting. Do you know why that's there?

public JPanel contentPane;
public File currentFile = null;

The first one is okay, though it forces you to create subpanes for any "side-by-side" editor views. The second one, not so much. For one you're locking youself into having only a single file open at one time, and secondly you're imbuing meaning to null. This could be remedied by something like the following:

List<LayoutUnit> layoutUnits = new ArrayList<>();

in these layout units you could then have a

List<StackableDisplayArea> displayStack = new ArrayList<>();
int currentlyActiveDisplayStack = 0;

a StackableDisplayArea would then be something like an EditorArea, a SettingsArea, a FolderViewArea, ... I hope you get the idea :)

public MainFrame instance;

This only makes sense as a static field. Anybody who can access this, already has the instance. You either have MainFrame foo = [...]; or you are inside MainFrame already.

In the former case, foo == foo.instance and in the latter case this.instance == this. Why do you have that member?


Sidebar: At this point, we're two screens into the class. I have not yet seen any conceivable reason for all the things I noted upon. I may be corrected later.

That being said, I could leave this standing as a whole review already. I do hope strongly, that you come back for more reviews with other parts of your code (and an improved version of what you're showing here).


this.instance = this;

WHY?

    /**
     * Menu action managers setup.
     */

Up until here the constructor was okay, albeit somewhat large. After that comment, the constructor takes the job of DependencyInjection and setting up your whole program. There is a single underlying issue to this:

Behavioural definitions are not separated from the visual definitions. Let me explain:

Circling back to the ActionManagers it seems that you have difficulty cleanly separating UI from Actions. I strongly recommend you look into common UI patterns like MVC, MVP and MVVM.
I personally had the best results in swing when using a "Presenter-First MVP" approach. Doing that allowed me to specify the behaviour of my program on an abstract level (as a presenter-interface). I then followed by creating a "View". that View is where all the Swing components reside. This ties back to the considerations above about StackedAreas. That's a display concern, it should be a separate class.

The final step is then to encapsulate the Data you're using in the whole construct into the "Model". You can repeat this process for each of the Components you have.

This allows you to pretty cleanly separate display concerns from behaviour concerns from data-housing concerns.

And that in turn allows you to reduce the scope of changes, the coupling between classes and increases maintainability and testability. All of that is vital when you're writing on a program for a long time. Months or even Years of programming are not inconceivable for a project such as yours. Especially on those timescales, making changes easy is a huge timesaver.

I'm not even going to comment on the rest of the constructor aside from the next paragraph. That constructor is currently the main of your program. Adressing the overarching design problems will radically change the constructor.

A constructor should only be responsible for constructing the class that it belongs to. It shouldn't wire together components. When your constructor becomes simply assigning members, you'll know that your design goal has been reached.

\$\endgroup\$
  • \$\begingroup\$ How do i use layout managers without awt imports? Cmon... \$\endgroup\$ – Krzysztof Szewczyk Feb 4 '18 at 15:51
  • \$\begingroup\$ Just read comment on instance to know why it's used... \$\endgroup\$ – Krzysztof Szewczyk Feb 4 '18 at 15:54

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.