1
\$\begingroup\$

I have made a connection method which returns TCPMasterConnection.

public Class Connect{

String ipAddress;
int port;
TCPMasterConnection tcpCon;

    public TCPMasterConnection OverTCP(String ipAddress, int port)
    {
      this.ipAddress = ipAddress;
      this.port = port;

      TCPMasterConnection tcpConnection = null;

    try {
        tcpConnection = new TCPMasterConnection(InetAddress.getByName(ipAddress));
    } catch (Exception e) {
        e.getMessage();
    }
    tcpConnection.setPort(port);
    try {
        tcpConnection.connect();
    } catch (Exception e) {
        e.printStackTrace();
    }
      return tcpConnection;
    }

    public void setTcpCon(TCPMasterConnection tcpCon) {
       this.tcpCon = tcpCon;
    }
}

And I have multiple methods which uses TCPMasterConnection:

public Class Write extends Connect{

public int[] OverTCP_16Bit_Int(int unitID, int startAddress, int dataToWrite int count) throws Exception
{
   this.unitID = unitID;
   this.startAddress = startAddress;
   this.dataToWrite = dataToWrite;
   this.count = count;

   SimpleRegister reg = new SimpleRegister(dataToWrite);
   Register[] registers = new Register[count];

   for(int i = 0; i<registers.length; i++)
   {
       registers[i] = reg;
   }

   WriteMultipleRegistersRequest write_mreq = new WriteMultipleRegistersRequest();

   write_mreq.setReference(startAddress);
   write_mreq.setUnitID(unitID);
   write_mreq.setRegisters(registers);

   ModbusTCPTransaction transaction = new ModbusTCPTransaction(tcpCon);
   transaction.setRequest(write_mreq);
   transaction.execute();
   return transaction.getResponse();
  }
   public void setTcpCon(TCPMasterConnection tcpCon) {
     this.tcpCon = tcpCon;
   }
 }
}

public Class Read extends Connect{

public int[] OverTCP_16Bit_Int(int unitID, int startAddress, int count) throws Exception
{

this.unitID = unitID;
   this.startAddress = startAddress;
   this.count = count;
   this.x = new int [count];
   this.Stringas = new String[count];

   ReadMultipleRegistersRequest read_mreq = new ReadMultipleRegistersRequest();

   read_mreq.setUnitID(unitID);
   read_mreq.setReference(startAddress);
   read_mreq.setWordCount(count);

   ModbusTCPTransaction transaction = new ModbusTCPTransaction(tcpCon);
   transaction.setRequest(read_mreq);
   transaction.execute();
   ReadMultipleRegistersResponse read_mres = (ReadMultipleRegistersResponse) transaction.getResponse();

   for (int n = 0; n < read_mres.getWordCount(); n++) 
   {
       x[n] = read_mres.getRegisterValue(n); 
   }
  return x
  }
   public void setTcpCon(TCPMasterConnection tcpCon) {
      this.tcpCon = tcpCon;
 }
 }
}

So in main I do this:

public static void main(String[] args) throws Exception {

  String ip = "127.0.0.1";
  int port = 502;

  Connect connect = new Connect();

  Read read = new Read();
  Write write= new Write();

  TCPMasterConnection connectTCP = null;

  Connect[] objectList = new Connect[2];

  objectList[0] = read;
  objectList[1] = write;

  connectTCP = connect.OverTCP(ipAddress, port);

  for(Connect x: objectList){
      x.setTcpCon(connectTCP);
  }
}

Is this a proper way to do this?

So my point is to make a proper way to transfer connection through multiple classes for each method they have. So I don't have to do something like this every time user wants to use different function.

Connection connect = new Connect
TCPMasterConnection connectTCP = connect.OverTCP("127.0.0.1", 502);
read.setTcpCon(connectTCP);
read.DoStuff();

write.setTcpCon(connectTCP);
write.DoStuff();

I made in main class so that it transfers connection but I don't know if it is a proper way and overall I want to improve my code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please, provide a little explanation about what your each part of code is doing (even if seem obvious). Also, try to provide all the code, if it is not huge (replace ... with the actual code) to obtain a better review. \$\endgroup\$ – Alexei Feb 20 '16 at 11:31
1
\$\begingroup\$

It may make sense to organize your objects in a different way. With Read and Write extending Connect, you are basically treating them as if they are connections rather than being operations performed over a connection.

I think the most straightforward way to model operations over a connection would be something like this:

public static class GameConnection {
    private TCPMasterConnection conn;

    public void connectTo(String ipAddress, int port) throws IOException {
        conn = new TCPMasterConnection(InetAddress.getByName(ipAddress));
        conn.setPort(port);
        conn.connect();
    }

    public int[] read(int unitID, int startAddress, int count) throws IOException {
        // Do the read over this.conn
    }

    public int[] write(int unitID, int startAddress, int dataToWrite, int count) throws IOException {
        // Do the write over this.conn
    }

    public void close() throws IOException {
        conn.close();
    }
}

To use it, you then just

    GameConnection conn = new GameConnection();

    conn.connectTo("127.0.0.1", 8080);
    int[] readStuff = conn.read(1, 2, 3);
    int[] writeStuff = conn.write(1, 2, 3, 4);
    doSomeOtherThingsWithIt(conn);
    conn.close();

For more abstraction, you could make the GameConnection an interface and have different implementations for different transport protocols.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Good job on your first answer. \$\endgroup\$ – SirPython Feb 20 '16 at 19:48
  • \$\begingroup\$ You should throw an illegal state exception in the read and write method if conn is not initialized \$\endgroup\$ – Johnny000 Feb 21 '16 at 9:27

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.