Ordering Gallery images by custom order

Question

I wrote some code a few weeks ago, to take a string similar to

seq[]=8&seq[]=7&seq[]=5&seq[]=1&seq[]=4&seq[]=3&seq[]=6&seq[]=0&seq[]=2

and put it into a gallery, to be ordered by jQuery UI's Sortable, which works all good, however, I'm concerned about the amount of times I .Replace the values in the string.

This code has been bugging me, because I'm employed to rewrite unethical code, and the few times I actually write code from scratch, it looks like this. So, basically, I would love your opinions on this code.

To clarify, the imgMgr is to get the JSON string, and the nodelist is for getting an XML doc of images from the imgMgr.

#region BuildGallery
private void BuildGallery()
{

    StringBuilder sb = new StringBuilder();

    sb.Append("<div class=\"controls\"><center><button class=\"js-serialize\" style=\"border: 0;height: 25px;background-color: #F4FA58;margin-bottom: 8px;\">Save Layout</button></center></div><div class=\"sortable\"><ul id=\"sortable\">");
    int pos = 0;
    string str = "";
    ImageManagerV1 imgMgr = new ImageManagerV1();
    str = imgMgr.galleryOrder(LocalConfig.ImageManagerAuthCode, Convert.ToInt32(hdnListingId.Value), "");
  //  str = "seq[]=8&seq[]=7&seq[]=5&seq[]=1&seq[]=4&seq[]=3&seq[]=6&seq[]=0&seq[]=2";
    str.Replace("seq[]=","-").Split('-');
    XmlNodeList nodelist = xmlDoc.SelectNodes("image");
    str = str.Replace("&", "");
    if (str == "") {
        for (int i = 0; i < nodelist.Count; i++) {
            str += "seq[]=" + i;
        }
    }
    string[] strNew = str.Replace("seq[]=", "-").Split('-');
        str = "";
        for (int i = 0; i < strNew.Length; i++) {
            if (strNew[i] != "") {
                str += "-" + strNew[i];
            }
        }
    if (nodelist.Count > strNew.Length) {
        for (int f = strNew.Length; f < nodelist.Count; f++) {
            str += "-" + f;
        }
    }
    int val2 = 0;
    string[] stringArray = str.Split('-');
    foreach (string num in stringArray) {
        var newNum = num.ToString();
        if (newNum.Length > 0) {
            val2 = Convert.ToInt32(newNum);
            XmlNode image = nodelist[val2];
            string thumbpath = image.Attributes["thumb"].Value;
            string detailpath = image.Attributes["detail"].Value;
            string fileName = detailpath.Substring(detailpath.LastIndexOf(@"/") + 1);
            sb.AppendFormat("<li class=\"thumb ui-state-default\" id=\"seq_{0}\">", val2);
            sb.AppendFormat("<img src=\"{0}\" alt=\"{2}\" title=\"Gallery: {2}\" width=\"90\" height=\"90\"/><a href=\"\" data=\"{4}\"class=\"makeHero\">Make Hero</a><a href=\"\" data=\"{4}\"class=\"deleteImage\">Del</a>", thumbpath, detailpath, listing.ListingName, pos, fileName);
            sb.Append("</li>");
        }
    }
    sb.Append("</ul><div class=\"clear\"></div></div>");
        if (sb.Length > 0)
        {
            litGallery.Text = string.Format("<div id=\"listing-gallery\" class=\"listing-gallery\">{0}</div>", sb.ToString());
        }
    }
#endregion

RubberDuck · Accepted Answer · 2015-01-06 01:42:06Z

string str = "";

There's no sense in assigning str to an empty string just to over write it two lines later. This would be much more terse.

ImageManagerV1 imgMgr = new ImageManagerV1();
string str = imgMgr.galleryOrder(LocalConfig.ImageManagerAuthCode, Convert.ToInt32(hdnListingId.Value), "");

str.Replace("seq[]=","-").Split('-');

Why not just split on "seq[]="? The Replace feels unnecessary here. Actually, the more I look at this, why are you splitting? Split returns an array, but you're not making an assignment here. I really think you need to decide what this line is supposed to do.

str = str.Replace("&", "");
if (str == "") {
    for (int i = 0; i < nodelist.Count; i++) {
        str += "seq[]=" + i;
    }
}

Two things. As a maintainer of the code, I've no idea why you're stripping out the ampersands. A comment here is in order. Secondly, this loop needs a StringBuilder. StringBuilder is a mutable string of characters and thus, more efficient in this situation.

string[] strNew = str.Replace("seq[]=", "-").Split('-');

Okay, now you can skip the Replace and split directly on "seq[]=". Speaking of, that string seems to show up an awful lot. You should replace it with a constant.

string[] separators = new string[] {"seq[]="};
strNew = str.Split(separators, SplitStringOptions.None);

    for (int i = 0; i < strNew.Length; i++) {
        if (strNew[i] != "") {
            str += "-" + strNew[i];

At first glance, I want to tell you to use a string builder again, but I honestly don't have the foggiest idea why you just split the string apart, just to put it back together. Forget what I just said about splitting, and just Replace("seq[]=","-"). Don't split. It looks to be unecessary.

@Quill for iterative reviews, it is recommended that you post your updated code as a new question. More people will see it that way. — Nick Udell
– Nick Udell, Commented Jan 6, 2015 at 12:38
I'm just not going to have time to come back to this, but here's the IDEOne I was working on. It should give you an idea of where I was heading. ideone.com/OWz2oP — RubberDuck
– RubberDuck, Commented Jan 6, 2015 at 20:40

Heslacher · Accepted Answer · 2015-01-06 09:09:11Z

you should make this method independent from any UI element like litGallery. In this way you are decoupling your code from UI dependencies.
instead of using the not commented class variable xmlDoc you should have passed it as input parameter.
your method should take a String and a XmlDocument input parameter and should return a String representing the html of the gallery.
by extracting these loooooong html strings like "<div class=\"controls\"><center><button class=\"js-serialize\" style=\"bord....... into meaningful constants the code building the gallery will be more readable.
the whole method should be broken into multiple methods which should
- preparing the str
- building the html
most C# developers like me prefer to assign String.Empty over ""
you should be consistent with the coding style you use. If you place the opening brace { at the same line with the if condition` you should stick to it.
the check fo if (sb.Length > 0) is not necessary and can be removed
calling ToString() on a String is not necessary
the newNum variable is not necessary and can be removed. You should just use num variable
the whole building of this string should be done using a List<T> instead of adding, replacing and splitting these strings.

\$\begingroup\$ You just saved me a ton of time. \$\endgroup\$

RubberDuck
– RubberDuck

2015-01-06 09:41:03 +00:00
Commented Jan 6, 2015 at 9:41 — RubberDuck
– RubberDuck, Commented Jan 6, 2015 at 9:41

Stack Exchange Network

Ordering Gallery images by custom order

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Ordering Gallery images by custom order

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions