1
\$\begingroup\$

based on this stackoverflow topic: https://stackoverflow.com/a/12150289/15270760 I have decided to create a simple NTP client displaying difference between NTP server response and DateTime.Now called right after receiving NTP response. I was just curious of the result. I have read both RFC's 2030 and 1305 and it appered to me that there are four different timestamps, not as I expected, one. I have decided to cover all four of them. I was just curious of the result, again. I humbly ask for a code review based on this snippet of code. At the beggining I would like to emphasise that this is a simple console application so please don't be too harsh on me for keeping the Main() method name.

public sealed class NTP { public static void Main() { // Source: https://stackoverflow.com/a/12150289/15270760 const string ntpServer = "time.windows.com"; var address = Dns.GetHostEntry(ntpServer).AddressList.FirstOrDefault(); if (address is null) throw new Exception("No ntp server found"); var udpClient = new UdpClient(); var ntpServerEndpoint = new IPEndPoint(address, 123); udpClient.Connect(ntpServerEndpoint); udpClient.Client.ReceiveTimeout = 3000; var tokenSource = new CancellationTokenSource(); Task.WaitAny(new Task[] { Task.Run(() => DisplayNtpServerResponsePeriodically(udpClient, ntpServerEndpoint, tokenSource)), Task.Run(() => ReadUserInput(tokenSource)), }); udpClient.Close(); } private static void ReadUserInput(CancellationTokenSource tokenSource) { var userInput = Console.ReadLine(); while (userInput != "q" && userInput != "Q") { userInput = Console.ReadLine(); } Console.WriteLine("Closing application..."); tokenSource.Cancel(); } private static async Task DisplayNtpServerResponsePeriodically(UdpClient udpClient, IPEndPoint ntpServerEndpoint, CancellationTokenSource tokenSource) { while (!tokenSource.Token.IsCancellationRequested) { var bytesBuffer = new byte[48]; // Based on RFC 2030: // Leap Indicator: First two bits - warning of an impending leap second to be inserted/deleted // Version Number: Middle three bits - indicating version number, 011 equal to IPv4 only // Mode values: Last three bits - indicating the mode, 011 equal to client mode bytesBuffer[0] = 0b00011011; udpClient.Send(bytesBuffer); bytesBuffer = udpClient.Receive(ref ntpServerEndpoint); var serverResponseReceivedDateTime = DateTime.Now; var ntpServerTimestamps = GetNtpTimeStamps(bytesBuffer); Console.WriteLine($"{ntpServerTimestamps.ToString(serverResponseReceivedDateTime)}"); await Task.Delay(5000); } } private static NtpServerTimestamps GetNtpTimeStamps(byte[] bytesBuffer) { var fieldsIndexes = new int[] { 16, 24, 32, 40 }; var timestampsArray = new DateTime[4]; for (var i = 0; i < fieldsIndexes.Length; i++) { ulong seconds = BitConverter.ToUInt32(bytesBuffer, fieldsIndexes[i]); ulong secondsFraction = BitConverter.ToUInt32(bytesBuffer, fieldsIndexes[i] + 4); seconds = ConvertToLittleEndianUnsignedLong(seconds); secondsFraction = ConvertToLittleEndianUnsignedLong(secondsFraction); var milliseconds = (seconds * 1000) + ((secondsFraction * 1000) / 0x100000000L); var dateTime = (new DateTime(1900, 1, 1, 0, 0, 0, DateTimeKind.Utc)).AddMilliseconds((long)milliseconds); timestampsArray[i] = dateTime; } return new NtpServerTimestamps(timestampsArray); } // Source: stackoverflow.com/a/3294698/162671 static uint ConvertToLittleEndianUnsignedLong(ulong x) { return (uint) (((x & 0x000000ff) << 24) + ((x & 0x0000ff00) << 8) + ((x & 0x00ff0000) >> 8) + ((x & 0xff000000) >> 24)); } private sealed class NtpServerTimestamps { private DateTime _referenceTimestamp { get; } private DateTime _originateTimestamp { get; } private DateTime _receiveTimestamp { get; } private DateTime _transmitTimestamp { get; } public NtpServerTimestamps(DateTime[] datetimeArray) { _referenceTimestamp = datetimeArray[0].ToLocalTime(); _originateTimestamp = datetimeArray[1].ToLocalTime(); _receiveTimestamp = datetimeArray[2].ToLocalTime(); _transmitTimestamp = datetimeArray[3].ToLocalTime(); } public string ToString(DateTime serverResponseReceivedDateTime) { var stringBuilder = new StringBuilder(); stringBuilder.AppendLine($"-------------------NTP Server Response-------------------"); stringBuilder.AppendLine($"Reference: {_referenceTimestamp}:{_referenceTimestamp.Millisecond}"); stringBuilder.AppendLine($"Originate: {_originateTimestamp}:{_originateTimestamp.Millisecond}"); stringBuilder.AppendLine($"Receive: {_receiveTimestamp}:{_receiveTimestamp.Millisecond}"); stringBuilder.AppendLine($"Transmit: {_transmitTimestamp}:{_transmitTimestamp.Millisecond}"); stringBuilder.AppendLine($"Now: {serverResponseReceivedDateTime}:{serverResponseReceivedDateTime.Millisecond}"); stringBuilder.AppendLine(""); stringBuilder.AppendLine($"Reference-Now difference: {(serverResponseReceivedDateTime - _referenceTimestamp).TotalMilliseconds}"); stringBuilder.AppendLine($"Originate-Now difference: {(serverResponseReceivedDateTime - _originateTimestamp).TotalMilliseconds}"); stringBuilder.AppendLine($"Receive-Now difference: {(serverResponseReceivedDateTime - _receiveTimestamp).TotalMilliseconds}"); stringBuilder.AppendLine($"Transmit-Now difference: {(serverResponseReceivedDateTime - _transmitTimestamp).TotalMilliseconds}"); return stringBuilder.ToString(); } } }
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Here are my observations

DisplayNtpServerResponsePeriodically

  • 0b00011011: I would suggest to move such magic number into a const/static readonly
  • I would also recommend to do not reuse outgoing buffer as an incoming buffer
  • Last but not least Task.Delay should also receive the CancellationToken to avoid waiting "unnecessary" 5 seconds
// Based on RFC 2030: // Leap Indicator: First two bits - warning of an impending leap second to be inserted/deleted // Version Number: Middle three bits - indicating version number, 011 equal to IPv4 only // Mode values: Last three bits - indicating the mode, 011 equal to client mode private static readonly byte[] Request_Bytes = new byte[] { 0b00011011 }; private static async Task DisplayNtpServerResponsePeriodically(UdpClient udpClient, IPEndPoint ntpServerEndpoint, CancellationTokenSource tokenSource) { while (!tokenSource.Token.IsCancellationRequested) { udpClient.Send(Request_Bytes); var bytesBuffer = udpClient.Receive(ref ntpServerEndpoint); var serverResponseReceivedDateTime = DateTime.Now; var ntpServerTimestamps = GetNtpTimeStamps(bytesBuffer); Console.WriteLine($"{ntpServerTimestamps.ToString(serverResponseReceivedDateTime)}"); await Task.Delay(5000, tokenSource.Token); } }
  • BTW: ntpServerTimestamps.ToString(serverResponseReceivedDateTime): here the ToString name feels really unnatural
    • Most of the time ToString can receive a formatter parameter
    • So, I would suggest to find some different name here

GetNtpTimeStamps

  • Yet again move the magic numbers into const/static readonly fields
    • Please find some better naming here, I've just extracted them from the method
private static readonly DateTime baseTime = new DateTime(1900, 1, 1, 0, 0, 0, DateTimeKind.Utc); private static readonly int[] fieldsIndexes = new int[] { 16, 24, 32, 40 }; private const int MilliSecondMultiplier = 1_000; private const long MilliSecondDivider = 0x100_000_000;
  • I would also suggest to use a List for timestamps rather than an array
    • It is much easier to use (.Add instead of [i])
    • It allows you to use foreach instead of for
  • You don't need to use brackets because of the operator precedence
private static NtpServerTimestamps GetNtpTimeStamps(byte[] bytesBuffer) { var timestamps = new List<DateTime>(fieldsIndexes.Length); foreach (var index in fieldsIndexes) { ulong seconds = BitConverter.ToUInt32(bytesBuffer, index); ulong secondsFraction = BitConverter.ToUInt32(bytesBuffer, index + 4); seconds = ConvertToLittleEndianUnsignedLong(seconds); secondsFraction = ConvertToLittleEndianUnsignedLong(secondsFraction); var milliseconds = seconds * MilliSecondMultiplier + secondsFraction * MilliSecondMultiplier / MilliSecondDivider; timestamps.Add(baseTime.AddMilliseconds((long)milliseconds)); } return new NtpServerTimestamps(timestamps.ToArray()); }

NtpServerTimestamps's ctor

  • It feels a bit clumsy to receive an array and then use it to retrieve items based on their positions
    • Maybe passing all four parameters separately is more explicit and less fragile

NtpServerTimestamps's ToString

  • If you introduce the following two helper methods
private string FormatDateTimeToDisplay(string prefix, DateTime timestamp) => $"{prefix}: {timestamp}:{timestamp.Millisecond}"; private string FormatDateTimesToDisplay(string prefix, DateTime received, DateTime timestamp) => $"{prefix}-Now difference: {(received - timestamp).TotalMilliseconds}";
  • And you are replacing the StringBuilder to a simple string.Join(Environment.NewLine, then your code would become much more readable
public string ToString(DateTime serverResponseReceivedDateTime) => string.Join(Environment.NewLine, new[] { "-------------------NTP Server Response-------------------", FormatDateTimeToDisplay("Reference", _referenceTimestamp), FormatDateTimeToDisplay("Originate", _originateTimestamp), FormatDateTimeToDisplay("Receive", _receiveTimestamp), FormatDateTimeToDisplay("Transmit", _transmitTimestamp), FormatDateTimeToDisplay("Now", serverResponseReceivedDateTime), "", FormatDateTimesToDisplay("Reference", serverResponseReceivedDateTime, _referenceTimestamp), FormatDateTimesToDisplay("Originate", serverResponseReceivedDateTime, _originateTimestamp), FormatDateTimesToDisplay("Receive", serverResponseReceivedDateTime, _receiveTimestamp), FormatDateTimesToDisplay("Transmit", serverResponseReceivedDateTime, _transmitTimestamp) });
  • Yet again the ToString name feels a bit weird here
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.