Simple wrapper for LocalReport

Question

I've built a wrapper for a LocalReport and I would like to hear your suggestions about it:

public class ReportWrapper : IDisposable
{
    private string _reportResource;
    private LocalReport _report = new LocalReport();

    private int m_currentPageIndex;
    private IList<Stream> m_streams;

    public Dictionary<string, object> ReportDataSource { get; private set; }

    public PrinterSettings printSettings { get; set; }

    /// <summary>
    ///     true, wenn die Seite im Querformat gedruckt werden soll, andernfalls false.
    ///     Der Standardwert ist false
    /// </summary>
    public bool Landscape { get; set; }

    public string FileName { get; set; }

    public ReportWrapper(string ReportResource)
    {
        this._reportResource = ReportResource;
        this.ReportDataSource = new Dictionary<string, object>();
        this._report = new LocalReport(); ;
    }

    public void Print()
    {
        _report.ReportEmbeddedResource = _reportResource;

        foreach (var item in ReportDataSource)
            _report.DataSources.Add(CreateReportDataSource(item.Key, item.Value));

        Export();
        Printing();
    }

    public void toPDF(string FilePath)
    {
        _report.ReportEmbeddedResource = _reportResource;

        foreach (var item in ReportDataSource)
            _report.DataSources.Add(CreateReportDataSource(item.Key, item.Value));

        var fi = new FileInfo(string.Format(CultureInfo.CurrentCulture, "{0}/{1}", FilePath, FileName));

        if (fi.Exists)
            fi = CreateNewFileName(fi);

        Byte[] results = _report.Render("PDF");
        File.WriteAllBytes(fi.FullName, results);
    }

    private static FileInfo CreateNewFileName(FileInfo fi)
    {
        int modifier = 1;

        while (true)
        {
            var tempFi = new FileInfo(string.Format(CultureInfo.CurrentCulture, "{0}/{1}({2}){3}", fi.Directory, Path.GetFileNameWithoutExtension(fi.Name), modifier, fi.Extension));

            if (!tempFi.Exists)
            {
                fi = tempFi;
                break;
            }

            modifier++;
        }

        return fi;
    }

    public static ReportDataSource CreateReportDataSource(string Name, object Source)
    {
        var bindingSource = new BindingSource();
        bindingSource.DataSource = Source;

        return new ReportDataSource(Name, bindingSource);
    }

    // Routine to provide to the report renderer, in order to
    //    save an image for each page of the report.
    private Stream CreateStream(string name, string fileNameExtension, Encoding encoding, string mimeType, bool willSeek)
    {
        Stream stream = new MemoryStream();
        m_streams.Add(stream);
        return stream;
    }

    // Export the given report as an EMF (Enhanced Metafile) file.
    private void Export()
    {
        string deviceInfo = @"<DeviceInfo>
                                <OutputFormat>EMF</OutputFormat>
                              </DeviceInfo>";
        Warning[] warnings;

        m_streams = new List<Stream>();

        _report.Render("Image", deviceInfo, CreateStream, out warnings);

        foreach (Stream stream in m_streams)
            stream.Position = 0;
    }

    private void PrintPage(object sender, PrintPageEventArgs ev)
    {
        Metafile pageImage = new Metafile(m_streams[m_currentPageIndex]);

        // Adjust rectangular area with printer margins.
        Rectangle adjustedRect = new Rectangle(
            ev.PageBounds.Left - (int)ev.PageSettings.HardMarginX,
            ev.PageBounds.Top - (int)ev.PageSettings.HardMarginY,
            ev.PageBounds.Width,
            ev.PageBounds.Height);

        // Draw a white background for the report
        ev.Graphics.FillRectangle(Brushes.White, adjustedRect);

        // Draw the report content
        ev.Graphics.DrawImage(pageImage, adjustedRect);

        // Prepare for the next page. Make sure we haven't hit the end.
        m_currentPageIndex++;
        ev.HasMorePages = (m_currentPageIndex < m_streams.Count);
    }

    private void Printing()
    {
        if (m_streams == null || m_streams.Count == 0)
            throw new Exception("Error: no stream to print.");

        PrintDocument printDoc = new PrintDocument();
        printDoc.PrinterSettings = printSettings;
        printDoc.DocumentName = FileName;
        printDoc.DefaultPageSettings.Landscape = Landscape;

        printDoc.PrintPage += new PrintPageEventHandler(PrintPage);
        m_currentPageIndex = 0;
        printDoc.Print();
    }

    public LocalReport getReport()
    {
        return _report;
    }

    public string ReportEmbeddedResource
    {
        get { return _reportResource; }
    }

    public SubreportProcessingEventHandler SubreportProcessing { get; set; }

    #region IDisposable

    private bool disposed = false;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        // Check to see if Dispose has already been called.
        if (!this.disposed)
        {
            // If disposing equals true, dispose all managed
            // and unmanaged resources.
            if (disposing)
            {
                _report.Dispose();
            }
        }
        disposed = true;
    }

    #endregion
}

Usage example:

   var rw = new ReportWrapper("name.space.to.MyReport.rdlc");
   rw.ReportDataSource.Add("Source1", someObject);
   rw.ReportDataSource.Add("Source2", someDetailobject);
   rw.SubreportProcessing += YourSubDataSource;

    public void YourSubDataSource(object sender, SubreportProcessingEventArgs e)
    {
        \\Logic    
    }

Answer 1

Naming

Either use m_ or _ or no prefix for your class variables, but don't mix the styles. Also if you use one of the first two, there is no need to use this.

Based on the naming guidelines method names should be named using PascalCase casing. Properties should be named using PascalCase casing too.

Method parameters should be named using camelCase casing.

---

Using braces {} for single line loop/if statements won't harm the application but makes it less error prone.

---

This

_report.ReportEmbeddedResource = _reportResource;

foreach (var item in ReportDataSource)
    _report.DataSources.Add(CreateReportDataSource(item.Key, item.Value));  

is duplicated code which should be extracted to a method PrepareReport().

---

Instead of using String.Format() for composing a filename, you should use Path.Combine(). The Path.Combine() method is doing some internal checking like if there are illegal characters in the path etc. which can come handy.

---

This condition

if (!tempFi.Exists)
{
    fi = tempFi;
    break;
}  

can be simplified to

if (!tempFi.Exists)
{
    return tempFi;
}  

---

The input parameters of the CreateStream() method aren't used at all, so you can safely remove them.

---

Comments should describe why something is done. Let the code speak for itself about what is done by using meaningful names for classes, methods and parameters.

Additional you shouldn't mix languages for commenting. Right now you have german and english comments. I would use english, because if a new developer is hired who doesn't speak german, you almost for sure can say that he/she can speak english.

---

Because you are calling the Export() method before you are calling the Printing() method you don't need the check for m_streams == null in the Printing() method.
Thinking about this, you don't need the check for m_streams.Count == 0 neither.

Answer 2

I haven't been on the site in a while so figured I'd give this one a go:

My first point is debatable and not essential but reflects my own preference.

Consider making the class sealed by default. Whenever I say this to people, most seem surprised. You're probably asking why, I mean, does it matter?

• Your class is not designed with inheritance in mind, it just isn't suitable for extending.
• Inheritance is rarely a suitable option anyway. I prefer composition myself.

For more on this, read here.

Regarding the class members, you're intermixing the _ and m_ conventions, neither of which I would follow anyway but it's a matter of preference. The important point is that you should pick one and remain consistent.

You are also accessing the members with the 'this' prefix which just seems wrong given you've already opted for the underscore convention (which is basically a counter to using 'this').

Choose between the following:

private int handle;
private int _handle;
private int m_handle;

public ClassName(handle)
{
    this.handle = handle;
    _handle = handle;
    m_handle = handle;
}

My preference is the no underscore and 'this' prefix. Rather than relying on a custom rule, I feel it's better to use the feature provided by the language.

Also, some class members have no newline between them and others do, which again, is inconsistent.

This:

private LocalReport _report = new LocalReport();

along with your constructor, means you can't inject the object you're wrapping and thus unit tests/mocking will be a pain. Actually, you're initialising this object twice. Once in the field declaration and then again in the constructor which is redundancy.

This:

public Dictionary<string, object> ReportDataSource { get; private set; }

I would expose as an IDictionary. That way, your implementation may change, but calling code shouldn't break. The calling code only ever cares about the public contract of an object, so it would be better to only expose that interface.

As @Heslacher pointed out, properties should use PascalCase and members camelCase. You should also be using braces for if/for/... statements. see here However, I still disapprove of placing the statement on the same line still.

I'd recommend renaming the function: Printing() To me, this suggests it is a continuous/ongoing action, which it is not.

This:

throw new Exception("Error: no stream to print.");

I don't see the point in prefixing the message with 'error'. You know it's an error as it's an exception.

// Export the given report as an EMF (Enhanced Metafile) file.
private void Export()

I'd rename this to be more specific. Instead of using a comment, consider calling it ExportAsMetaFile. Comments should be used rarely, and if so, describe the why rather than the what. You can expect any reasonable programmer to be able to understand what the code is doing, but sometimes you may have to explain an odd decision, in which case only, I would consider adding a comment.

I'd move the following out into a separate function in which you pass the dependency:

foreach (Stream stream in m_streams)
{
     stream.Position = 0;
}

Which indicates why you're setting them back to zero. Currently you're export method is doing more than it should. see here (I also apply this to functions)

I'd split the following across multiple lines to aid readability:

var tempFi = new FileInfo(string.Format(CultureInfo.CurrentCulture, "{0}/{1}({2}){3}", fi.Directory, Path.GetFileNameWithoutExtension(fi.Name), modifier, fi.Extension));

The parameters on CreateStream aren't used.

In Export, if deviceInfo isn't supposed to change, then show this intent by making it constant.

I would move the following:

// Adjust rectangular area with printer margins.
    Rectangle adjustedRect = new Rectangle(
        ev.PageBounds.Left - (int)ev.PageSettings.HardMarginX,
        ev.PageBounds.Top - (int)ev.PageSettings.HardMarginY,
        ev.PageBounds.Width,
        ev.PageBounds.Height);

Into a separate function describing what it is doing, perhaps AppendPrinterMargin?. Also, that way, it can be used elsewhere if need be.

Functions such as:

public LocalReport getReport()
{
    return _report;
}

Would be more C# idiomatic as properties:

public LocalReport Report { get { return _report; } }

Lastly:

// If disposing equals true, dispose all managed
// and unmanaged resources.
if (disposing)
{
    _report.Dispose();
}

This comment is incorrect. If disposing is true, you should only dispose of managed resources. unmanaged resources should be disposed regardless of this boolean flag (I am aware, that you only have references to managed resources), but this is important. managed resources must be disposed when disposing is true as you have done.

I hope this was helpful :)

Answer 3

In addition to the already existing reviews a few points:

1. Try to avoid generic Exceptions. A more specific exception is better because otherwise you force the caller to always catch all Exceptions when they might only want to deal with a very specific one. An InvalidOperationException might be more suited in your case.

2. toPDF is not a very good name. Methods named like toFoo i'd expect to return the object as Foo. SaveToPDF would convey the intend better.

3. The class owns a List<> of Stream. Stream is IDisposable and hence you should dispose of the Streams in the list.

4. I was a bit confused by your usage of m_streams and how the whole Export stuff is put together so I searched a bit and found this MSDN page which your implementation seems to be very heavily based on. I'm not convinced this is the best way to go about it. Essentially it exports the report into a bunch of Streams which then get printed.

   In the end you have a whole set of class members which means that multiple calls to Print will probably yield strange results. I think the design can be changed so that neither the LocalReport nor the list of streams would be required as class members but rather as transient members being passed around between functions. This would then allow you to:

   1. Make your implementation not require to be IDisposable
   2. Allow you to call Print multiple times on the same object without ill side effects.

   In the end copying complex examples from MSDN is not always a good idea from a design point of view as their examples are trying to show the usage of methods rather than convey good design.