Problem
I’ve written a basic program to generate a script that will automatically configure routing in a Cisco router. It reads an input file which contains the directly connected interfaces of a router.
e.g. of the file. It can be generated by writing show ip route connected in the router itself.
C 192.168.1.0/30 is directly connected, FastEthernet0/0
C 192.168.1.8/30 is directly connected, FastEthernet0/1
C 192.168.2.0/24 is directly connected, Ethernet0/3/0
It will generate a command that includes all the ports and save it as a file.
import java.io.File;
import java.io.FileNotFoundException;
import java.io.PrintStream;
import java.util.Scanner;
public class CNRG
{
static String fileName;
static File inputFile;
static PrintStream write;
static int [] subnetMask = new int[4];
public static void main(String[] args) throws FileNotFoundException
{
int userChoice;
String ip;
init();
pickRoutingProtocol();
}
private static void pickRoutingProtocol() throws FileNotFoundException
{
Scanner Input = new Scanner(System.in);
int userChoice;
System.out.println("Pick your routing protocol"
+ "n1. OSPF v2"
+ "n2. EIGRP v4");
userChoice = Input.nextInt();
switch(userChoice)
{
case 1:
write.println("enable");
write.println("conf t");
write.println("router ospf 1");
break;
case 2:
write.println("enable");
write.println("conf t");
write.println("router eigrp 1");
break;
}
generateCIDR(userChoice);
}
private static void generateCIDR(int userChoice) throws FileNotFoundException
{
Scanner Read = new Scanner(new File(fileName+".txt"));
boolean done = false;
String line, ipcidr, ip, port;
int cidr;
ip = " ";
subnetMask[0] = 0;
subnetMask[1] = 0;
subnetMask[2] = 0;
subnetMask[3] = 0;
while(!done)
{
if(Read.hasNextLine())
{
line = Read.nextLine();
Scanner Scan = new Scanner(line);
Scanner Scan2 = new Scanner(line).useDelimiter(",");
Scan.next();//no
ipcidr = Scan.next();//ip and cidr
Scan2.next();// no
port = Scan2.next();//port
//Splits the ipcidr line and stores it in an array
String[] s = ipcidr.split("/");
ip = s[0];
cidr = Integer.parseInt(s[1]);
//re initialize array
subnetMask[0] = 0;
subnetMask[1] = 0;
subnetMask[2] = 0;
subnetMask[3] = 0;
while(cidr <= 32 && cidr >= 25)
{
subnetMask[3] += 1;
cidr--;
}
while(cidr <= 24 && cidr >= 17)
{
subnetMask[2] += 1;
cidr--;
}
while(cidr <= 16 && cidr >= 9)
{
subnetMask[1] += 1;
cidr--;
}
while(cidr <= 8 && cidr >= 1)
{
subnetMask[0] += 1;
cidr--;
}
generateSubnetMask();
generateWildCardMask(userChoice, ip);
}
else
{
done = true;
}
}
write.println("end");
}
private static void generateSubnetMask()
{
for (int i = 0; i < 4; i++)
{
switch(subnetMask[i])
{
case 8:
subnetMask[i] = 255;
break;
case 7:
subnetMask[i] = 254;
break;
case 6:
subnetMask[i] = 252;
break;
case 5:
subnetMask[i] = 248;
break;
case 4:
subnetMask[i] = 240;
break;
case 3:
subnetMask[i] = 224;
break;
case 2:
subnetMask[i] = 192;
break;
case 1:
subnetMask[i] = 128;
break;
default:
subnetMask[i] = 0;
break;
}
}
}
private static void generateWildCardMask(int userChoice, String ip)
{
if(userChoice == 1)//ospf
{
write.println("network " + ip + " "+ (255-subnetMask[0]) +"."
+(255-subnetMask[1]) +"."
+(255-subnetMask[2]) +"."
+(255-subnetMask[3]) +" area 51");
}
else//eigrp
{
write.println("network " + ip + " " +(255-subnetMask[0]) +"."
+(255-subnetMask[1]) +"."
+(255-subnetMask[2]) +"."
+(255-subnetMask[3]));
}
}
public static void init()
{
fileName = "R1";
inputFile = new File(fileName + ".txt");
try
{
write = new PrintStream(fileName + "_script.txt");
}
catch (FileNotFoundException ex)
{
ex.printStackTrace();
}
}
}
the output would be something like this (depending on protocol)
enable
conf t
router ospf 1
network 192.168.1.0 0.0.0.3 area 51
network 192.168.1.8 0.0.0.3 area 51
network 192.168.2.0 0.0.0.255 area 51
end
The improvements that I thought are
-
connect the router and program in a pipeline of sort, maybe use a scripting language? Thus, automate the program for each router without having the need to store the router’s output as a file.
-
Somehow similar to the first idea. Execute the output directly and bypass the file writing process.
Solution
In a few words…
Your code as it stands is very very hard to unit test as it relies on global state, on system.in and on writing to a file directly and pretty much all methods have side-effects. The indentation is far from perfect.
It also feels very imperative. As you are using a OO language, you should have multiple objects that delegate work to each other.
By using dependency injection you could then make your objects read from something that is not System.in
(a file or a string for example) and write to something that is not always the same file. This’d make unit testing easier as well.
To sum my previous points : your code really feels “scripty”.
Zooming in a little bit
3 variables are never used and some variable start with an upper letter which don’t follow java conventions.
public class CNRG
{
What does CNRG mean ? ^^’ Please search for the good practices in naming classes 😉
fileName
is only written once so it should be final
… actually it should be a constant. Same goes for the inputFile
field.
The extension is also part of the filename IMO.
So in the end that’d be :
public static final String FILE_NAME = "R1.txt";
public static final File INPUT_FILE = new File(FILE_NAME);
write
is a poor name for a field. The PrintStream
may actually never be closed which is also a problem if you were to write a bigger program.
As mentionned earlier in my post, most static methods should be put into their own class and turned to object method.
It also feels off to have the userChoice
stored as an int
an not as an enum which would more closely matches the fact that it’s a protocol.
enum Protocol {
OSPF_V2, EIGRP;
}
It’d also remove some useless comments such as if(userChoice == 1)//ospf
as the code would now look like if (protocol == Protocol.OSPF_V2)
which is more readable.
generateCIDR
does way too much and should be refactored almost entirely.
Finally,
while(!done)
{
if(Read.hasNextLine())
{
// ...
else
{
done = true;
}
}
…can be simplified to :
while(read.hasNextLine())
{
// ...
}
Controlling the input
If the user input neither 1 nor 2, then what happens ?
Never trust user input 😉
Magic number (& cie)
You use quite a lot of magic numbers though the code which is usually a bad idea.
generateSubnetMask()
: this method is pure black magic to me ; you should comment or try to split it in easier to get methods.
Example of code with DI
Here is a sample of what it could look like (code is not complete) :
public class Launch {
public static final String FILE_NAME = "R1.txt";
public static final File INPUT_FILE = new File(FILE_NAME);
private static CiscoRouterConfigurationAuthor.Protocol pickRoutingProtocol() {
// TODO read user input and determine which protocol to use
}
public static void main(final String[] args) throws IOException {
try (BufferedWriter bw = Files.newBufferedWriter(INPUT_FILE.toPath())) {
// TODO initialize reader
String conf = new CiscoRouterConfigurationAuthor(pickRoutingProtocol(), reader).getConfiguration();
bw.write(conf);
}
}
}
public class CiscoRouterConfigurationAuthor {
private static final int MASK_SIZE = 4;
private static final String NEW_LINE = System.lineSeparator();
public enum Protocol {
OSPF_V2, EIGRP;
}
private final Protocol protocol;
private final Reader inputReader;
private final int[] subnetMask;
public CiscoRouterConfigurationAuthor(final Protocol protocol, final Reader inputReader) {
// TODO : control argument
this.protocol = protocol;
this.inputReader = inputReader;
this.subnetMask = new int[MASK_SIZE];
}
String header() {
return "enable" + NEW_LINE + "conf t";
}
public String getConfiguration() {
StringBuilder sb = new StringBuilder();
sb.append(header() + NEW_LINE);
switch (protocol) {
case OSPF_V2:
sb.append("router ospf 1");
break;
case EIGRP:
sb.append("router eigrp 1");
break;
}
sb.append(NEW_LINE);
sb.append(generateCIDR());
sb.append(NEW_LINE);
return sb.toString();
}
// TODO : rest of the methods
As we have greatly reduced the number of side-effects, you can now unit test your code. For example, one unit test can look like :
public class CiscoRouterConfigurationAuthorTest {
@Test
public void getConfiguration_ShouldReturnCorrectOSPF_V2ConfigurationWhenGivenValidParameter() {
StringReader sr = new StringReader("value to test for");
CiscoRouterConfigurationAuthor cr = new CiscoRouterConfigurationAuthor(Protocol.OSPF_V2, sr);
assertEquals(expected, cr.getConfiguration());
}
}
And what do you expect from us ? I suppose that your outputs are correct and that you expect some remarks about the structure of your program.
So given that you want to extend it it will be better to refactor it. Think about responsibilities, you have one input something that manipulate this input and one output.
The input is a file with lines that contains informations about Connection
. You can already create a Connection
class and a ConnectionParser
. This will be easy to test and write with a regex.
class ConnectionParser implements Iterator<Connection> {
private static final Pattern LINE = Pattern.compile("C\s+(?<source>(?<ip>[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})/(?<port>[0-9]+))(?:.*),\s+(?<target>.*)");
private final Iterator<Connection> iterator;
public ConnectionParser(InputStream in) {
iterator = new BufferedReader(new InputStreamReader(in)).lines().map(LINE::matcher)
.filter(Matcher::matches)
.map(this::parse)
.iterator();
}
}
On the ouput you have a format that is dedicated to a Protocol
. The output vary with the protocol but some parts are identical. Your new abstract class Protocol
is an application of the template pattern.
abstract class Protocol {
public void prepare(PrintWriter out) {
out.println("enable");
out.println("conf t");
out.println("router ospf 1");
}
public abstract void write(Connection connection, PrintWriter out);
public void finish(PrintWriter out) {
out.println("end");
}
}
Then you have the generator itself that needs a ConnectionParser
and a Protocol
to do his work. In fact the generator will drive the Protocol
to prepare the output, write all Connection
s and finish, it is an application of the builder pattern.
class Generator {
private final ConnectionParser parser;
private final Protocol protocol;
public Generator(final ConnectionParser parser, final Protocol protocol) {
this.protocol = protocol;
this.parser = parser;
}
public void writeTo(OutputStream out) throws IOException {
PrintWriter writer = new PrintWriter(out); // PrintWriter is easier to work than raw output
protocol.prepare(writer);
while ( parser.hasNext() ) {
protocol.write(parser.next(), writer);
}
protocol.finish(writer);
writer.flush();
}
}
Then you’r done. You are able to create unit tests for each components and create an intergation test to verify that the generator can read and write with each protocol.
The latest part is the execution of it where you would like to run with one command. This is a simple parsing of something like generate input.txt -f ospf > output.ospf.txt
Later if you want to connect either the input or ouptut to your routers, you just have to create another ConnectionParser
and replace the stream of Generator#writeTo(OutputStream)
with an higher level component (URL ?) or just pipe your output and/or input.