10
\$\begingroup\$

I am trying to create something that is very reusable here so that I can use it in other sites for other pages etc.

This code is for a simple photo gallery,

I took the code from here, but I changed it a bit and would like to add more functionality to it eventually, though.

PhotoGallery.ascx

<%@ Control Language="C#" AutoEventWireup="true" CodeFile="PhotoGallery.ascx.cs" Inherits="PhotoGallery" %>
<style>
    #ImageGallery {
        overflow:auto; 
        height:500px; 
        width:330px; 
        display:block;
    }
    #GalleryContainer {
        width:300px;
        align-content:center;
        border-right:3px solid #EEEEEE;
    }
    .thumbnails {
        cursor:pointer;
        width:100px;
    }
</style>
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script>
<script type="text/javascript">
    $(document).ready(function () {
        $('#ImageGallery img').click(function () {
            var bigImagePath = $(this).attr('src');
            $('#bigImage').attr('src', bigImagePath);
        });
    });
</script>
<h3>Before and After Pictures</h3>
<table>
    <tr>
        <td id="GalleryContainer">
            <div id="ImageGallery">
                <asp:Repeater ID="Repeater1" runat="server">
                    <ItemTemplate>
                        <img class="thumbnails" src='<%#Eval("ImageUrl") %>' alt='<%#Eval("ImageUrl") %>' />
                    </ItemTemplate>
                </asp:Repeater>
            </div>
        </td>
        <td>
            <img id="bigImage" alt="" />
        </td>
    </tr>
</table>

PhotoGallery.ascx.cs

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Web;
using System.Web.UI;
using System.Web.UI.WebControls;

public partial class PhotoGallery : System.Web.UI.UserControl
{
    protected void Page_Load(object sender, EventArgs e)
    {
        if (!IsPostBack)
        {
            PopulateImages();
        }
    }

    public string FolderPath {get;set;}

    private void PopulateImages()
    {
        List<Image> myImages = new List<Image>();
        DirectoryInfo DI = new DirectoryInfo(Server.MapPath(FolderPath));
        foreach (var file in DI.GetFiles())
        {
            myImages.Add(new Image { ImageUrl = FolderPath + file.Name });
        }
        Repeater1.DataSource = myImages;
        Repeater1.DataBind();
    }
}
\$\endgroup\$
  • \$\begingroup\$ You would like to add more functionality, how does that relate to CodeReview? \$\endgroup\$ – Abbas Dec 25 '14 at 13:26
  • 1
    \$\begingroup\$ If the code is not maintainable it is usually not extendable either. \$\endgroup\$ – Malachi Dec 25 '14 at 15:04
4
\$\begingroup\$
  • the variable holding the DirectoryInfo object should be named using lower case.

  • you should always use Sytem.IO.Path.Combine() to combine a folder with a filename.

  • you could use DirectoryInfo.EnumerateFiles combined with the Select() and assign the returned IEnumerable to the DataSource property of the Repeater control.

  • you should add spacing to the property FolderPath. Instead of

    public string FolderPath {get;set;}  
    

    you should use

    public string FolderPath { get; set; }  
    
  • Repeater1 is an ugly name which isn't decsriptive either. Change it to something more descriptive.

\$\endgroup\$
2
\$\begingroup\$

You really just need an IEnumerable of strings and not images. You could probably directly bind the results of the GetFiles method!!

Also I don't really see a need to break the method out of Page_Load. Too many submethods leads to spaghetti.

Generally when dealing with IEnumerables there is no need to new up a list and add to it, just call a linq method on the IEnumerable you want to transform.

\$\endgroup\$
  • 1
    \$\begingroup\$ I disagree. I think that having the method do all the work is the right thing to do here. Page_Load is like Main it should only have the logic it absolutely needs, so we separate out the logic into methods that do certain things. like the PopulateImages method \$\endgroup\$ – Malachi Jan 2 '15 at 15:04
  • \$\begingroup\$ For what the code is right now, I think it's overkill to apply that kind of rule, especially because it is a void method that changes the page's state which can become a big maintenance headache. If OP had a static pure method to gen the collection I would support that. I also could support the method as-is if it was being called more than once. \$\endgroup\$ – moarboilerplate Jan 2 '15 at 15:17
  • \$\begingroup\$ how does it change the page state? it's called only on the initial page load. Are you saying that the PopulateImages method needs to be static? \$\endgroup\$ – Malachi Jan 2 '15 at 15:25
  • \$\begingroup\$ It calls DataBind. Especially if the page becomes more complex with more methods and databound controls, a buried DataBind in a method can easily become problematic. As for the static method, no, I'm not suggesting PopulateImages be static, it can't be because it has control references. I'm suggesting a pure method that takes in the results of GetFiles and returns an IEnumerable of images (though in this case there's no need as OP doesn't need to generate any images). That kind of method can be unit tested directly and if needed broken out into a layer without having to extract UI references. \$\endgroup\$ – moarboilerplate Jan 2 '15 at 15:45
  • 1
    \$\begingroup\$ "Too many submethods leads to spaghetti." Really? I just can't get on board with that. \$\endgroup\$ – RubberDuck Jan 2 '15 at 15:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.