feat(eventfactory): Event Factory and UserEventFactory #174
Conversation
|
|
||
| public SnapshotEvent(string entityId, string uuid, string key, long timestamp, int? revenue, long? value, EventTags eventTags) | ||
| public SnapshotEvent(string entityId, string uuid, string key, long timestamp, int? revenue = null, float? value = null, EventTags eventTags = null) |
There was a problem hiding this comment.
How come SnapshotEvent doesn't get a Builder?
There was a problem hiding this comment.
you are right, I will add builder for it, initially I decided not to have builder for simple classes but this class have same type of properties which may be interchanged. I actually missed it. Will add
| /// <summary> | ||
| /// Helper to compute Unix time (i.e. since Jan 1, 1970) | ||
| /// </summary> | ||
| protected static long SecondsSince1970 |
There was a problem hiding this comment.
This seems like it could be a more global utility
| @@ -0,0 +1,200 @@ | |||
| /* | |||
| * Copyright 2017-2019, Optimizely | |||
| @@ -0,0 +1,200 @@ | |||
| /* | |||
There was a problem hiding this comment.
I don't think there is a need for making things internal anymore. We want to be able to expose as much of the SDK as possible. Plus the class itself it public and not protected :)
There was a problem hiding this comment.
I will remove it from Internals, just FYI, class should have internal attribute not to expose outside of assembly.
| /// <summary> | ||
| /// UserEventFactory builds ImpressionEvent and ConversionEvent objects from a given UserEvent. | ||
| /// </summary> | ||
| public class UserEventFactory |
There was a problem hiding this comment.
Is there a strong reason for making this a separate file? I was thinking a generic "Event" factory can create all these types of events
There was a problem hiding this comment.
I think it's a categorization between userevents which are impression and conversion and server event which is only LogEvent. Same in java. that's why I did it.
There was a problem hiding this comment.
Ok that is fine then.
OptimizelySDK/Utils/GeneralUtils.cs
Outdated
| @@ -0,0 +1,18 @@ | |||
| using System; | |||
There was a problem hiding this comment.
This can be less generic. We can call it DateUtils
Summary
Test plan