Weather Slack bot implementation in .NET w/ location support #2

ghost · 2017-08-06T13:34:26Z

See PR #1 for previous version including feedback

This updated version includes support for locations, so now the bot

Responds to weather now, weather tomorrow, and what should I wear?
- Location needs to have been set for the user
Responds to weather now <location>, weather tomorrow <location>
- needs to be a city state pair, where state is a two letter abbreviation e.g. New York, NY
User can set location with set me to <location>
- needs to be a city state pair, where state is a two letter abbreviation e.g. New York, NY

To support locations, we needed to support persistence so that we can carry location settings across sessions. For this PR, I went with a small SQLite database for ease of deployment, and setup. See the code and README for more details on considerations.

* Rework w/ new requirement and feedback

s-machina

@kdt4242 Thanks for taking round two at this. This is more the direction we were looking for, and we greatly appreciate the effort.

I've left a few questions to prompt some more discussion and expect @alan-vaughn to chime in as well.

s-machina · 2017-08-07T21:57:19Z

SlackBotPrototype/SlackBotPrototype.sln

@@ -0,0 +1,40 @@
+
+Microsoft Visual Studio Solution File, Format Version 12.00


Is it common in C# project to check in sln files. I imagine almost all such projects use Visual Studio, so it might be. In Java land, we used to keep project files out of version control since it was essentially personal preference as to what to use.

Yes, from what I'm familiar with for the reason you hypothesized. I think if we wanted a definitive answer we could poke around Github and see the trends (.sln included or not)

s-machina · 2017-08-07T22:13:14Z

SlackBotPrototype/DBPreparer/locations-seed.json

@@ -0,0 +1,9002 @@
+[


Out of curiosity, how did you generate this? It's rather thorough. I would have been happy with like, five cities to get the point across.

Also, are the lat/longs airports? Centroids?

I initially started out with just three cities but felt lame when playing around with it. I considered hooking up an actual service, decided against it and looked for "major cities united states coordinates". The data source I ended up with was a public gist (referenced in the README): https://gist.github.com/Miserlou/c5cd8364bf9b2420bb29 . I wrote a quick script to read it in, map to my model, add the abbreviations, and serialize to this file.

The original data source didn't mention if these were centroids or not. Since you prompted, I plugged a few samples into Google and they look to be centroids. We could cross reference the entries against another data source that are known centroids to be certain.

s-machina · 2017-08-07T22:15:04Z

SlackBotPrototype/DBPreparer/Program.cs

+				db.CreateTable<User>();
+				db.CreateTable<Location>();
+
+				// SHORTCUT: Normally the source of the


Useful comment, and sensible.

s-machina · 2017-08-07T22:16:31Z

SlackBotPrototype/API/SqLitePersistence.cs

+			}
+		}
+
+		// SHORTCUT: We skip testing this because it's hard to test this in isolation, but it contains


What do you mean by testing in isolation here?

I was referencing the fact that I skipped integration tests using SQLite since this code directly interacts via SQL. Definitely doable and worthwhile considering logic in the query.

s-machina · 2017-08-07T22:17:36Z

SlackBotPrototype/API/SqLitePersistence.cs

+	// a file per entity. Keep them together for simplicity here.
+	// 
+	// Also skipping tests for these because we'd need to set up integration tests
+	// with a SQLite database. If we weren't using a embedded file DB like SQLite, testing


What are the challenges of setting up integration tests with SQLite?

This is not a comprehensive list, but what comes top of mind for me is:

Ensuring accurate state of DB between individual tests and test runs

Could reset DB between each run

Or create a new DB for each test run

Depending on data required for tests, size of DB could be a factor especially if we go one DB/test -- would affect test speed, build times, deploy times

Would have to properly clean up DBs artifacts from tests otherwise your build server won't be happy

s-machina · 2017-08-07T22:26:22Z

SlackBotPrototype/API/DarkSkyWeatherProvider.cs

+		{
+
+			var timeStamp = when.DateTimeToUnixTimestamp();
+			// SHORTCUT: We could cache requests in memory for recent forecast requests so we don't have


Caching on something that changes as infrequently as weather is a worthwhile endeavor, methinks.

In general, where would you look to apply caching? It seems like it could muddle up the implementation of an external service implementation, but would be generally useful for many services.

Agreed, caching makes sense here especially considering the Dark Sky API is metered. If we're using a cloud service that charges per request we can see the cost savings easily.

Since IO is notoriously a performance bottleneck, anything that does IO is a prime candidate for caching (network accessed services, file access, DB access). At the end of the day, caching is an optimization that adds complexity (invalidation logic) so often it's better to wait until we identify performance issues before making the trade off.

s-machina · 2017-08-07T22:30:27Z

SlackBotPrototype/API/MessageParser.cs

+
+namespace API
+{
+	public class MessageParser : IMessageParser


Is this still used?

No, I missed deleting this when I was doing cleanup.

s-machina · 2017-08-08T05:38:33Z

SlackBotPrototype/SlackBotPrototype/Program.cs

+			Console.Read();
+		}
+
+		private static void OnClientOnOnMessageReceived(NewMessage message)


This is an amazing method name.

Agree with what you're alluding to. This should be renamed to something like OnClientMessageReceived. Amazingly I missed this during all my passes through the code.

It's why we do code reviews. :-)

s-machina · 2017-08-08T05:52:38Z

SlackBotPrototype/SlackBotPrototype/Program.cs

+			_weatherProvider = new DarkSkyWeatherProvider(darkSkyToken, new RestClientHttpProvider(ConfigConstants.DarkSkyBaseUrl));
+			_messageToCommandConverter = new MessageToCommandConverter(_weatherProvider, new SqLitePersistence());
+
+			_client.Connect((connected) => {


Up to this point, I see configuration and initialization of the connection to slack. Makes sense. The closure (as the comment states) is called once the connection is established.

What happens after this? We get a user list. We add a message handler. There is setting and waiting. Give me a brief overview of how we're interacting with the library after we've established the connection.

Once connection is established, the manual reset event is signaled and the thread continues so we can actual start handling events coming in.

This library uses .NET events, we subscribe to the event with our handler.

The actual events originate from Slack via web sockets, the library handles bubbling up the web socket events to our .NET events ending up in the handler.

Our handler processes the message, and if a response is required we call the library method PostMessage which publishes back to Slack via web sockets.

alan-vaughn · 2017-08-08T16:40:07Z

SlackBotPrototype/API.Tests/EmbeddedResourceTestBase.cs

+
+namespace API.Tests
+{
+	public class EmbeddedResourceTestBase


since this doesn't use any instance variables or configure any other aspect of the class, should we look at just having it be either a mixin or a utility function? what are the downsides or upsides of using a class to do this?

Yes, both make sense in this case for the reason you specified as alternatives for code sharing. Some considerations

In C# we can only inherit one class, so we're preventing ourselves from inheriting another class -- which might have state.

By inheriting we're making an assuming that all future additions will be relevant for all inheritors. If we later added instance variables or methods that don't apply, we end with bloated object with unnecessary functionality.

By inheriting we can limit the scope of our functions using protected, this limits access to our function more than a public utility method would. So caller's know that if they want to use this method, they have to inherit which means that the subclass has a relationship with the base class and it's functionality.

If we used a public utility method, we're saying the caller can more freely use the method in different places without necessarily considering the relationship.

alan-vaughn · 2017-08-08T16:44:50Z

SlackBotPrototype/API.Tests/EmbeddedResourceTestBase.cs

+			using (var file = assembly.GetManifestResourceStream(resourceName))
+			{
+				if (file == null)
+					Assert.Fail();


i would throw here, instead of using Assert not sure how it works in c# but some test suites will report test failures different from unexpected exceptions, and not having an expected fixture file is probably in the latter category

Good intuition, Assert.Fail() actually just ends up throwing an AssertionException (it has some additional context handling).

alan-vaughn · 2017-08-08T16:45:21Z

SlackBotPrototype/API.Tests/EmbeddedResourceTestBase.cs

+
+				using (var sr = new StreamReader(file))
+				{
+					var json = sr.ReadToEnd();


nit: i don't think the thing its parsing has to be json. i would just do return sr.ReadToEnd()

👍 Good insight.

alan-vaughn · 2017-08-08T16:56:37Z

SlackBotPrototype/API/SqLitePersistence.cs

+
+			using (var db = new SQLiteConnection(ConfigConstants.DbName))
+			{
+				return db.Query<Location>($"SELECT * FROM Location WHERE UPPER(City) = '{city.ToUpper()}' AND UPPER(StateAbbreviation) = '{state.ToUpper()}'").FirstOrDefault();


hmm, our of curiosity what is the default, that could be returned?

If there isn't a record, it'll return null. This means we end up having to deal with the the null propagation upstream. Alternatively we could

Handle null check here and throw an exception, and upstream catch the exception.

Call First() instead which will throw an exception if there's no record, and handle exception upstream.

At a high level, I think it's a choice between if we want to check for exceptions upstream or check for nulls.

kdt4242 and others added 8 commits July 23, 2017 13:12

Initial commit

bd0d562

Update README.md

eb6b83a

Update README.md

6ca913e

Update README.md

8860a00

Add @channel mention and name linking

80f5cfa

Rework from feedback (#1)

b592269

* Rework w/ new requirement and feedback

Update readme

1750f20

Documentation.

3a621a6

s-machina reviewed Aug 8, 2017

View reviewed changes

alan-vaughn reviewed Aug 8, 2017

View reviewed changes

		@@ -0,0 +1,40 @@

		Microsoft Visual Studio Solution File, Format Version 12.00

Weather Slack bot implementation in .NET w/ location support #2

Are you sure you want to change the base?

Weather Slack bot implementation in .NET w/ location support #2

Uh oh!

Conversation

ghost commented Aug 6, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-machina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost commented Aug 6, 2017 •

edited by ghost

Loading