patternjavaModerate
Enigma Machine Simulation
Viewed 0 times
machineenigmasimulation
Problem
This is an Enigma Machine Simulator written in Java:
Enigma.java
```
package enigma;
import java.io.*;
import java.awt.*;
import java.awt.event.*;
import java.io.IOException;
import enigma.rotor.LargeRotor;
import enigma.rotor.MedRotor;
import enigma.rotor.SmallRotor;
public class Enigma extends Frame {
String FileExtension = ".enigma";
public static void main (String[] args) {
screen = new Enigma();
screen.show();
}
public static final int FrameWidth = 660;
public static final int FrameHeight = 400;
public static Enigma screen;
private final Insets ins;
private SmallRotor smrotor = new SmallRotor();
private MedRotor medrotor = new MedRotor();
private LargeRotor lgrotor = new LargeRotor();
protected TextArea Message = new TextArea();
protected TextArea Encrypted = new TextArea();
protected TextField messFN = new TextField();
protected TextField encFN = new TextField();
protected Label FNLabel = new Label("Enter a FileName",Label.CENTER );
protected Label FNLabel2 = new Label("Enter a FileName",Label.CENTER);
public Enigma() {
this.FileExtension = ".enigma";
setTitle ("Enigma Simulator");
setSize (FrameWidth, FrameHeight);
setResizable(false);
setLayout(new FlowLayout());
add(Message);
Panel p = new Panel();
p.setLayout(new GridLayout(5,1));
p.add(new ButtonAdapter("Encrypt"){@Override
public void pressed(){Encrypt();}});
p.add(FNLabel);
p.add(messFN);
p.add(new ButtonAdapter("Load"){@Override
public void pressed(){LoadMessage(messFN.getText());}});
p.add(new ButtonAdapter("Save"){@Override
public void pressed(){SaveMessage(messFN.getText());}});
add(p);
add(Encrypted);
Panel p2 = new Panel();
p2.setLayout(new GridLayout(5,1));
p2.add(new ButtonAdapter("Decrypt"){@Override
public void pressed(){Decrypt();}});
p2.add(FNLabel2);
p2.add(encFN);
p2.add(new But
Enigma.java
```
package enigma;
import java.io.*;
import java.awt.*;
import java.awt.event.*;
import java.io.IOException;
import enigma.rotor.LargeRotor;
import enigma.rotor.MedRotor;
import enigma.rotor.SmallRotor;
public class Enigma extends Frame {
String FileExtension = ".enigma";
public static void main (String[] args) {
screen = new Enigma();
screen.show();
}
public static final int FrameWidth = 660;
public static final int FrameHeight = 400;
public static Enigma screen;
private final Insets ins;
private SmallRotor smrotor = new SmallRotor();
private MedRotor medrotor = new MedRotor();
private LargeRotor lgrotor = new LargeRotor();
protected TextArea Message = new TextArea();
protected TextArea Encrypted = new TextArea();
protected TextField messFN = new TextField();
protected TextField encFN = new TextField();
protected Label FNLabel = new Label("Enter a FileName",Label.CENTER );
protected Label FNLabel2 = new Label("Enter a FileName",Label.CENTER);
public Enigma() {
this.FileExtension = ".enigma";
setTitle ("Enigma Simulator");
setSize (FrameWidth, FrameHeight);
setResizable(false);
setLayout(new FlowLayout());
add(Message);
Panel p = new Panel();
p.setLayout(new GridLayout(5,1));
p.add(new ButtonAdapter("Encrypt"){@Override
public void pressed(){Encrypt();}});
p.add(FNLabel);
p.add(messFN);
p.add(new ButtonAdapter("Load"){@Override
public void pressed(){LoadMessage(messFN.getText());}});
p.add(new ButtonAdapter("Save"){@Override
public void pressed(){SaveMessage(messFN.getText());}});
add(p);
add(Encrypted);
Panel p2 = new Panel();
p2.setLayout(new GridLayout(5,1));
p2.add(new ButtonAdapter("Decrypt"){@Override
public void pressed(){Decrypt();}});
p2.add(FNLabel2);
p2.add(encFN);
p2.add(new But
Solution
Single responsibility principle
A class should be responsible for one thing. The
Start by moving everything out of the class that doesn't need a graphical element. Only keep buttons, panels, action listeners in
Use as much memory as you need and not more
In this code, the size of the
Isn't 10000 too much? Is it enough? You can eliminate such concerns by allocating exactly as much as you need:
Code style
The formatting is really awful. I suggest to copy paste into an IDE like Eclipse or similar, use the auto-format feature, and see what that looks like. That's the format most java developers expect to see and find it easy to review.
Naming
The convention in Java is to use
A class should be responsible for one thing. The
Enigma class is doing too much. It paints a graphical user interface, it works with files, it encrypts and decrypts, and lots of other things. It would be good to split this up aggressively.Start by moving everything out of the class that doesn't need a graphical element. Only keep buttons, panels, action listeners in
Enigma. And then rename the class to EnigmaGUI or similar to better reflect its primary (and hopefully, the only) responsibility.Use as much memory as you need and not more
In this code, the size of the
cypher array appears to be arbitrary:plain = plain.toUpperCase();
char [] cypher = new char[10000];
for(int i = 0; i < plain.length();i++)
{
cypher[i] = EncryptChar(plain.charAt(i));
}Isn't 10000 too much? Is it enough? You can eliminate such concerns by allocating exactly as much as you need:
char[] cypher = new char[plain.length()];Code style
The formatting is really awful. I suggest to copy paste into an IDE like Eclipse or similar, use the auto-format feature, and see what that looks like. That's the format most java developers expect to see and find it easy to review.
Naming
The convention in Java is to use
camelCase for variable and method names, and SHOUT_CASE for constants. It would greatly improve readability if you follow that.Code Snippets
plain = plain.toUpperCase();
char [] cypher = new char[10000];
for(int i = 0; i < plain.length();i++)
{
cypher[i] = EncryptChar(plain.charAt(i));
}char[] cypher = new char[plain.length()];Context
StackExchange Code Review Q#95089, answer score: 18
Revisions (0)
No revisions yet.