4
\$\begingroup\$

I'm working on a programming assignment which simulates network traffic between two hosts. My task is to implement the transport layer, the code for the other layers, GUI etc. is already provided. I have managed to do this and the code works according to the specifications listed below. My concern is that the code feels badly structured and could be improved significantly both in regards of structure and functionality.

Video demonstration of a working implementation from our instructors.

Requirements:

  • Uses the constructors values for timer and window size.
  • Sends packets that arrive from the applicationlayer if there's space in the window.
  • Buffers packets which arrives from the application layer if there's no space in the window.
  • Resends all packets that has been sent but not ACK:ed in case of a timer interrupt.
  • Responds to correct packets that arrive in order with an ACK.
  • Responds to correct packets that has been previously received with an ACK.
  • Ignores packets that are incorrect and/or arrives in the wrong order.
  • Sends packets from the buffer if an ACK has been received and there's space in the window.

Below is my code for the transport layer. I'm not sure if the rest of the simulator's code (which is not written by me and should not be reviewed) is needed to do a review but I will include a link to it anyway.

TransportLayer.java:

package protocolsimulator;
import java.util.HashMap;


import protocolsimulator.Segment;

public class TransportLayer
{      
    private LayerSimulator mLayerSimulator = null;    
    private String mId;    
    private int mTimerValue, mWindowSize;
    private int seqNumber = -1;
    private int ackNumber = -1;
    private int expected = 0;

    private HashMap<Integer, Segment> sentSegments = new HashMap<Integer, Segment>();
    private HashMap<Integer, Segment> receivedACKS = new HashMap<Integer, Segment>();
    private HashMap<Integer, Segment> recvBuffer = new HashMap<Integer, Segment>();
    private HashMap<Integer, Segment> sendBuffer = new HashMap<Integer, Segment>();


    /**
     * Consructs a TransportLayer
     *
     * @param id Id for the host
     * @param layers Class that simulates layers arount the Transport Layer
     */

    /**
     * Constructs a TransportLayer
     *
     * @param id Id for the host (A or B)
     * @param layers Simulates the layes above and below the Transport Layer
     * @param timerValue Timer value in ms (1000 ms = 1 sec)
     * @param windowSize The amount of unACK:ed packets we can transmit
     */
    public TransportLayer(String id, LayerSimulator layers, int timerValue, int windowSize){

        mLayerSimulator = layers;          
        mId = id;      
        mTimerValue = timerValue;
        mWindowSize = windowSize;

    }

    /**
     * Called from the Application Layer when a message is sent to an other host
     *
     * @param message Message from the Application Layer
     */
    public void toTransportLayer(String message){

        if(mId.equals("A")){
            seqNumber++;
        }
        else if(mId.equals("B")){
            ackNumber++;
        }

        System.out.println("Received: " + message + " from the application layer");
        Segment seg = new Segment (mId,seqNumber,ackNumber,message);

        if(mWindowSize > 0){
            mWindowSize--;
            if(!sendBuffer.containsValue(seg)){
                sendBuffer.put(seg.seqNumber, seg);
            }
            send(seg);
        }
        else{
            System.out.println("Window full. Adding " + seg.payload + " to buffer.");
            if(!sendBuffer.containsValue(seg)){
                sendBuffer.put(seg.seqNumber, seg);
            }
        }
    }

    /**
     * Called from the Network Layer when a segment arrives
     *
     * @param segment Segment that arrives from the Network Layer
     */
    public void toTransportLayer(Segment segment)
    {

        // Checks if the segment is corrupted..

        if(segment.isCorrect()){

            // Receiving from B..

            if(mId.equals("A")){

                System.out.println(mId +" received ACK: " + segment.payload);
                System.out.println("Timer stopped..");
                mLayerSimulator.resetTimer();
                mWindowSize++;
                System.out.println("Sendbuffer size: " + sendBuffer.size());

                if(!receivedACKS.containsValue(segment)){
                    receivedACKS.put(segment.ackNumber, segment);
                    expected = receivedACKS.size();
                    System.out.println("Received ACKS: " + receivedACKS.size());
                }

                if(receivedACKS.size() == sendBuffer.size()){
                    System.out.println("All packets received!");
                }

                if(sendBuffer.size() > receivedACKS.size() && mWindowSize > 0){

                    Segment next = sendBuffer.get(segment.ackNumber+1);
                    send(next);
                }

            }

            // Receiving segments from A and sending acks.

            else if(mId.equals("B")){

                System.out.println( mId + " received: " + segment.toString());

                // Check if segment has been received before.

                if(recvBuffer.containsKey(segment.seqNumber)){

                    System.out.println("Already in buffer: " + segment.payload);
                    sendACK(segment);

                }

                else if(segment.seqNumber <= expected){

                    System.out.println("Not in recvbuffer, adding to recvBuffer and sends to application layer.");
                    mLayerSimulator.toApplicationLayer(segment.payload);
                    recvBuffer.put(segment.seqNumber, segment);
                    expected = recvBuffer.size();
                    sendACK(segment);
                }

                else{
                    System.out.println("Wrong sequence number: " + segment.seqNumber + ", expected " + expected + ", ignoring..");
                }


            }


        }

        else{
            System.out.println(mId + " received a corrupt packet: " + segment.toString());
        }

    }

    /**
     * Function for sending acks
     */
    public void sendACK(Segment s){

        Segment ack = new Segment();
        ack.seqNumber = -1;
        ack.from = "B";
        ack.ackNumber = s.seqNumber;
        ack.payload = "ACK " + s.payload.charAt(0);
        System.out.println( mId + " sent ACK: " + s.payload);
        mLayerSimulator.toNetworkLayer(ack);

    }

    /**
     * Called by the simulator when the specified time for the timer has passed
     */
    public void timerInterrupt(){

        System.out.println("TimerInterrupt: Clearing window and resending unacked segments..");
        Segment segment;
        mWindowSize = 2;

        for(int i = 0; i < sentSegments.size(); i++){
            segment = sentSegments.get(i);
            if(segment != null && !checkACK(segment) && mWindowSize > 0){
                mWindowSize--;
                send(segment);
            }
        }
    }

    /**
     * Checks if segment has been acked before.
     */
    private boolean checkACK(Segment s){

        if(receivedACKS.containsKey(s.seqNumber)){
            return true;
        }
        else{
            return false;
        }
    }


    /**
     * Sends a segment to the network layer. Stores the segment in sentSegments.
     */
    private void send(Segment s){

        System.out.println(mId + " sends " + s.toString());

        if(!sentSegments.containsValue(s)){
            sentSegments.put(s.seqNumber,s);
        }

        mLayerSimulator.toNetworkLayer(s);
        mLayerSimulator.startTimer(mTimerValue);
    }
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

This doesn't look right:

private int seqNumber = -1;
private int ackNumber = -1;
private int expected = 0;

I'd expect expected to be initialized with a value of 0, so the = 0 is redundant. The two -1 defaults are off-putting though: it makes it look like 0 would be a valid ackNumber/seqNumber, and my instinct if telling me "nope". If there's a reason to initialize these two at -1, a comment is in order.


public void sendACK(Segment s){

    Segment ack = new Segment();
    ack.seqNumber = -1;
    ack.from = "B";
    ack.ackNumber = s.seqNumber;
    ack.payload = "ACK " + s.payload.charAt(0);
    System.out.println( mId + " sent ACK: " + s.payload);
    mLayerSimulator.toNetworkLayer(ack);

}

Why hard-code the -1 here? Was the seqNumber field meant to be that default value? If so, the field should probably be static final... but it seems odd that a "sequence number" would be a constant, hard-coded or not.

Also, if I'm reading this right, you're reporting success before you actually send the acknowledgement. Either change "sent" to "sending", or move that System.out.println statement after the line that's actually doing the work. Otherwise you could be reporting success even when toNetworkLayer throws an exception, which seems likely given it's presumably accessing a network.


private void send(Segment s){

    System.out.println(mId + " sends " + s.toString());

    if(!sentSegments.containsValue(s)){
        sentSegments.put(s.seqNumber,s);
    }

    mLayerSimulator.toNetworkLayer(s);
    mLayerSimulator.startTimer(mTimerValue);
}

Again, "sends" is a confusing wording. I'd go with a "sending..." wording, and then you might have a problem when toNetworkLayer throws an exception and you've already added the Segment to your sentSegments. And if it's already in the sentSegments, is it normal and intended that you re-send it?

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the reply! seqNumber and ackNumber is set to -1 since I used arrayLists before, making the different buffers start at position 0. They could be set to 0 now instead. -1 (or 0) also indicates if its an ack to a segment or a "regular" segment. A segment with seqNumber -1 and ackNumber 4 is an ack segment for the segment with seqNumber 4. I will move my "sends/sending" outputs to after I have actually done the sending. \$\endgroup\$
    – hajp
    May 27, 2015 at 20:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.