Minimalistic analytics library used by Dart Server and try.dartlang.org

dart_microlytics/lib/channels.dart

Index: dart_microlytics/lib/channels.dart
diff --git a/dart_microlytics/lib/channels.dart b/dart_microlytics/lib/channels.dart
new file mode 100644
index 0000000000000000000000000000000000000000..2fb00fa79d2f843f1b452e10b9e944fb6dd2fbfd
--- /dev/null
+++ b/dart_microlytics/lib/channels.dart
@@ -0,0 +1,53 @@
+// Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.

[ahe, 2014/09/02 13:44:23]

Add line between copyright and library declaration.

[lukechurch, 2014/09/02 19:52:36]

Done.

+library microlytics.channels;
+
+import 'dart:async';
+
+abstract class Channel {
+  void sendData(String data);
+  void shutdown() {}
+}
+
+/// [Channel] that implements a leaky bucket
+/// algorithm to provide rate limiting.
+/// See http://en.wikipedia.org/wiki/Leaky_bucket

[ahe, 2014/09/02 13:44:23]

Period.

[lukechurch, 2014/09/02 19:52:37]

That will cause the URL to break, I'll use parens instead

+class RateLimitingBufferedChannel extends Channel {
+  final List<String> _buffer = <String>[];
+  final Channel _innerChannel;
+  int _bufferSizeLimit;

[ahe, 2014/09/02 13:44:23]

Is this value going to change over the lifetime of this object?

[lukechurch, 2014/09/02 19:52:36]

There's a language feature interaction problem here.

If this is set to final, it can't be used with the optional params, as they may
not start with an _. This also interferes with the initializer syntax.

I've solved all of these problems by removing the _privacy.

[ahe, 2014/09/03 08:51:31]

You can write:

  RateLimitingBufferedChannel(
      this._innerChannel,
      {int bufferSizeLimit: 10, double packetsPerSecond: 1.0})
      : _bufferSizeLimit = bufferSizeLimit {

[lukechurch, 2014/09/03 11:27:16]

Done.

+  Timer _timer;

[ahe, 2014/09/02 13:44:23]

Same question.

[lukechurch, 2014/09/02 19:52:36]

No, but, making this final would imply moving the whole of the setup code into
initializer which harms readability. 

[ahe, 2014/09/03 08:51:31]

The current code isn't readable, as you're mixing up initializing the object
with Java-style constructors.

However, you can't make this field final without introducing a factory, and that
is probably overkill. On the other hand, it really stinks to create an object in
a bad state.

[lukechurch, 2014/09/03 11:27:16]

Acknowledged.

+
+  RateLimitingBufferedChannel(
+      this._innerChannel,
+      { bufferSizeLimit: 10, double packetsPerSecond: 1.0}) {

[ahe, 2014/09/02 13:44:23]

bufferSizeLimit isn't typed.

[ahe, 2014/09/02 13:44:23]

One line per parameter.

[lukechurch, 2014/09/02 19:52:36]

Done.

+

[ahe, 2014/09/02 13:44:23]

Remove extra line.

[lukechurch, 2014/09/02 19:52:36]

This makes is really hard to see where the init ends and the body starts, esp
after splitting the line above into two.

I've made the adjustment, but this doesn't seem like a good idea IMHO. Is it
mandated in the style guide? I think I use quite a bit more whitespace by
default than you do.

[ahe, 2014/09/03 08:51:30]

I can live with the extra line in those cases. However, it isn't the style that
we have adopted elsewhere, and I expect that some people on our team would
complain.

As for finding the start of the body, the real problem is that your constructor
is too complicated. If you stick to simple constructors you don't have this
problem. In addition, there is a difference in indentation that should help you
find the body.

[lukechurch, 2014/09/03 11:27:16]

Acknowledged.

+    if (!(packetsPerSecond > 0)) {
+      throw new ArgumentError("packetsPerSecond must be positive");

[ahe, 2014/09/02 13:44:23]

Period.

Most people with a long education probably knows that zero isn't a positive
number, but I suspect that many people would prefer to be told that
packetsPerSecond must be larger than zero.

[lukechurch, 2014/09/02 19:52:36]

Good point. Done.

+    }
+    this._bufferSizeLimit = bufferSizeLimit;

[ahe, 2014/09/02 13:44:23]

Use initializer.

[lukechurch, 2014/09/02 19:52:36]

Done.

+
+    int transmitDelay = (1000 / packetsPerSecond).floor();
+    _timer = new Timer(new Duration(milliseconds: transmitDelay), () {
+      _onTimerTick();
+    });
+  }
+
+  void _onTimerTick() {
+    if (_buffer.length > 0) {
+      String item = _buffer.removeLast();
+      _innerChannel.sendData(item);
+    }
+  }
+
+  void sendData(String data) {
+    if (_buffer.length >= _bufferSizeLimit) return;
+    _buffer.add(data);
+  }
+
+  void shutdown() {
+    _timer.cancel();
+    _innerChannel.shutdown();
+  }
+}