Changes and Improvements

[suhasks123]

1. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/main.go#L46

   Ideally, we would have a goroutine running here to listen for termination signals, like SIGTERM when the user presses ctrl+c. Once a termination signal is received, there can be a message that the server sends to the clients for a graceful exit instead of 'Server disconnected...Connection closed' which is sent in the event of an abrupt termination of the server process.

   This graceful exit can be achieved using the context package, where you have a main context for this goroutine. The server Run() function can then derive a new context from this main context. This context can be passed to the goroutines for handling
   the separate clients. When a termination signal is received, the main context can be cancelled, which will cause the derived server context to be cancelled as well. The client handling goroutines can then appropriately terminate the connections by sending a termination message to the individual clients. Once all the clients are disconnected, the server process can end.

2. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/server/server.go#L66

   The list of clients is a map, so it can be accessed directly right? There is no need to loop through the map to obtain the connection object of the required client.

3. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/server/server.go#L251

   As mentioned earlier, derive a new context from the main context that is taken as a parameter here. The main context can exclusively be used to handle termination signals.

4. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/server/server.go#L267

   Why is the server password being displayed here?

5. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/client/client.go#L150

   A prompt '>>' displayed when asking for the user input should be added here. Doesn't really make a functional difference but it's good user interface design for a command-line application.

6. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/client/client.go#L164

   I know its designed this way for the developer's convenience but maybe there are better formats for taking inputs? Characters like '~' aren't used much in general so asking the user to type these out might not be a good idea. No need to make this change now since it's a demo application and making this change requires some modification to how you parse your input
   but keep it in mind for future implementations. Special characters are fine for protocols but for user inputs, the format needs to be as simple as possible.

7. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/client/client.go#L235

   Shouldn't the client ask for the IP address as well? This approach is fine for use of the application as a demo but in a more practical situation, the server and client are not running on the same machine. In that situation, the client needs to ask the IP address of the target server. Keep 0.0.0.0:4545 as default for now but do ask for the IP address of the server along with the port number in the format <IP-address>:<port-number>. This can be directly given to the net.Dial() function instead of prepending it with :. Also, an option for the user to enter '-' for the default target IP address and port number would be nice.

8. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/client/client.go#L243

   I understand why this goto statement exists but I believe that there are better ways of achieving the same behavior(like a loop). It doesn't cause that much of a problem here since it is a smaller codebase but in larger codebases, it's tricky to visualize the control flow of the code which ultimately hurts its readability. The only reasonable place to use goto is in repeated error handling code or code that needs to be executed before a function returns. This is mostly in C and Golang has better mechanisms for this(defer), so there is pretty much no reason to use goto.

9. https://github.com/rak108/Distributed-DNS/blob/509bcd93649557b24e7388066eafa5dfcafea52e/learning_phase/go-chat/client/client.go#L245

   Any reason why the authentication is not included in the connection loop above? The user can be asked the password repeatedly until he gives the correct password right?