-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP/QRCoder2] Fluent API experimentation PR #558
base: master
Are you sure you want to change the base?
Conversation
|
||
namespace QRCoder.Builders.Payloads.Implementations | ||
{ | ||
public class EmailPayload : PayloadBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sample where the ctor only has the email address, and the subject/body are provided by extension methods. Realistically, I'm thinking that the required or common parameters always appear in the constructor (or as an overload), but all of the optional/rare options (like MailEncoding
) are extension methods.
@@ -0,0 +1,14 @@ | |||
namespace QRCoder.Builders.Payloads.Implementations | |||
{ | |||
public class StringPayload : PayloadBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use this class for any payload that immediately serializes to a string value. At least if we design it like CreateUrl(...)
. Of course it would be nearly as easy to have additional classes for each payload type, similar to how it is now, if we want new QRCodeBuilder.Url(...)
or whatnot.
@@ -0,0 +1,21 @@ | |||
namespace QRCoder.Builders.Payloads | |||
{ | |||
public abstract class PayloadBase : IPayload, IConfigurableEccLevel, IConfigurableEciMode, IConfigurableVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that while most all payloads can inherit from PayloadBase
and inherit some configurable settings, perhaps some payloads, like the swiss banking payload, does not support configuring the Eci mode or whatever. Or, we can override some of these properties to add validation, for instance ensuring that a certain minimum ECC level is maintained. There's a number of ways to actually implement this, all with similar end results.
public abstract class PayloadBase : IPayload, IConfigurableEccLevel, IConfigurableEciMode, IConfigurableVersion | ||
{ | ||
protected virtual QRCodeGenerator.ECCLevel EccLevel { get; set; } = QRCodeGenerator.ECCLevel.Default; | ||
QRCodeGenerator.ECCLevel IConfigurableEccLevel.EccLevel { get => EccLevel; set => EccLevel = value; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These interfaces are always explicitly implemented so that they do not appear on the intellisense list of accessible members for the class. Rather, the extension method will appear, which uses generics to return the same type.
If that's confusing, for example, this code would break the fluent syntax:
public PayloadBase WithEciMode(EciMode value)
{
_eciMode = value;
}
Because it returns the base class type (PayloadBase
), rather than the type of the derived type (e.g. WiFiPayload
). Extension methods allow the intended behavior, while eliminating duplicate code across each payload class.
public static T RenderWith<T>(this IPayload payload) | ||
where T : IRenderer, new() | ||
{ | ||
var renderer = new T(); | ||
renderer.Payload = payload; | ||
return renderer; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public interface IStreamRenderer | ||
{ | ||
MemoryStream ToStream(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept is that all byte-array-type renderers (or ones that can serialize to a byte array) implement this interface. If they use a MemoryStream
under the hood, it's a direct pass-through. If they generate a byte array directly, then wrapping it in a MemoryStream
is quite inexpensive, as it does not duplicate the entire byte array.
public static byte[] ToArray(this IStreamRenderer streamRenderer) | ||
{ | ||
var memoryStream = streamRenderer.ToStream(); | ||
#if NETSTANDARD || NETCOREAPP // todo: target .NET Framework 4.6 or newer so this code path is supported | ||
// by using TryGetBuffer, there is extremely small consequence to wrapping a byte[] in a MemoryStream temporarily | ||
if (memoryStream.TryGetBuffer(out var buffer) && buffer.Count == buffer.Array.Length) | ||
{ | ||
return buffer.Array; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here we can extract the original array without duplicating it.
public interface ITextRenderer | ||
{ | ||
string ToString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept is that all string-based renderers (Ascii, Svg) implement this interface. Another option would be to have it implement ToStringBuilder
instead, more similar to IStreamRenderer
above. However, we can't create an extension method called ToString
because object.ToString()
would always take precedence over an extension method.
QuietZone, _iconBackgroundColor); | ||
} | ||
|
||
public MemoryStream ToStream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renderers which return a specialized type may optionally also implement IStreamRenderer
as is shown here. Then ToFile
and other extension methods for byte-array type renderers are available.
|
||
protected bool QuietZone { get; set; } = true; | ||
bool IConfigurableQuietZones.QuietZone { get => QuietZone; set => QuietZone = value; } | ||
IPayload IRenderer.Payload { set => QrCodeData = value.ToMatrix(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to store the IPayload
object itself. I opted to call ToMatrix
immediately, so if there were any exceptions while converting the payload to a module matrix, it would be caught at this point, enhancing the ability to diagnose faulty fluent syntax.
return Convert.ToBase64String(data.Array, data.Offset, data.Count); | ||
} | ||
|
||
public static void ToFile(this IStreamRenderer streamRenderer, string fileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if any renderer wanted to optimize something like ToFile
, they could implement the method directly, which takes precedence over extension methods.
|
Hi Shane, thanks for the PR. That looks pretty interesting. However, I don't want to merge this into v1.x, but would like to include it in v2 first, as I don't want to make/document/communicate any more major API changes to v1. I hope that's okay with you. |
Certainly. I wrote this more of a discussion/preview/review of a potential approach. |
Summary
Experimentation with fluent API syntax
Samples
Intellisense samples
Preliminary Findings
new QRCodeBuilder.Email("test@microsoft.com")
rather thanQRCodeBuilder.CreateEmail("test@microsoft.com")
- which is pretty similar to the existingnew PayloadGenerator.Wifi()
syntax actually, with the difference being the fluent syntax for configuration. One benefit of usingnew
is that additional payloads can be added by simply adding a new class into the correct namespace. Whereas methods cannot be added toQRCodeBuilder
outside of QRCoder.RenderWith<T>
breaks the intellisense pattern because T does not give a list of the specific renderers available. Probably better to create an extension method for each renderer, likeRenderAsAscii()
andRenderAsPng(20)
insteadRenderWith<T>
does not allow for render-specific constructor parameters. I've compensated with support for only two patterns: (1) no arguments (2) pixels per module argument. HavingRenderAs...()
dedicated methods would allow each renderer to require specific arguments.RenderWith<T>
, as noted in item 2 above. Another reason to useRenderAsAscii()
or similar.ToArray
,ToStream
,ToFile
, etc) are really cool -- each renderer need only implementToStream
and all the other methods are available via extension methods. Similar functionality for text renderers; even with only implementingToString
they can also provideToFile
,ToStream
,ToBase64
, and so on via extension methods.