Random image selection



  • So on one of the sites I maintain, there's a background image that selected randomly when the page loads. There are so many things about this that I just hate:

    public partial class _Default : System.Web.UI.Page
    {
        protected void Page_Load(object sender, EventArgs e)
        {
            Int32 n = RandomNumber(1, 6);
    
            string strClassName = string.Empty;
    
            switch (n)
            {
                case 1:
                    //execute code block 1
                    strClassName = "divImage1";
                    break;
                case 2:
                    //execute code block 2
                    strClassName = "divImage2";
                    break;
                case 3:
                    //execute code block 2
                    strClassName = "divImage3";
                    break;
                case 4:
                    //execute code block 2
                    strClassName = "divImage4";
                    break;
                case 5:
                    //execute code block 2
                    strClassName = "divImage5";
                    break;
            }
    
    
            this.divImage.Attributes.Clear();
            this.divImage.Attributes.Add("class", strClassName);
    
    
        }
    
        private int RandomNumber(int min, int max)
        {
            Random random = new Random();
            return random.Next(min, max);
        }
    }
    

    As usual, this has not been anonymised in any way. This entire snippet is verbatim. While the jewel of shit in this turd crown is clearly the [code]RandomNumber()[/code] wrapper, don't forget:

    • meaningless variable names
    • a complete misunderstanding Hungarian notation
    • what would be code duplication in comments, but didn't get updated in the copy-paste



    Here's my replacement.

    <body style='color:#000000; background:#ffffff; '>
    protected void Page_Load(object sender, EventArgs e)
    {   
        string backgroundImage = "divImage" + new Random().Next(1, 6).ToString();
    
    <span style='color:#800000; font-weight:bold; '>this</span><span style='color:#808030; '>.</span>divImage<span style='color:#808030; '>.</span>Attributes<span style='color:#808030; '>.</span>Clear<span style='color:#808030; '>(</span><span style='color:#808030; '>)</span><span style='color:#800080; '>;</span>
    <span style='color:#800000; font-weight:bold; '>this</span><span style='color:#808030; '>.</span>divImage<span style='color:#808030; '>.</span>Attributes<span style='color:#808030; '>.</span>Add<span style='color:#808030; '>(</span><span style='color:#800000; '>"</span><span style='color:#0000e6; '>class</span><span style='color:#800000; '>"</span><span style='color:#808030; '>,</span> backgroundImage<span style='color:#808030; '>)</span><span style='color:#800080; '>;</span>
    

    }



  • A note for the non-.Net people: Random.Next(min, max) the minimum is inclusive and the max is exclusive so you don't get a missed case in the switch statement.



  • @locallunatic said:

    A note for the non-.Net people: Random.Next(min, max) the minimum is inclusive and the max is exclusive so you don't get a missed case in the switch statement.


    To be clear: this does work. It's just fucking stupid.


  • Considered Harmful

    @mikeTheLiar said:

    @locallunatic said:

    A note for the non-.Net people: Random.Next(min, max) the minimum is inclusive and the max is exclusive so you don't get a missed case in the switch statement.


    To be clear: this does work. It's just fucking stupid.

    I like:

                case 3:
                    //execute code block 2
    ...
                case 4:
                    //execute code block 2
    ...
                case 5:
                    //execute code block 2
    


  • @mikeTheLiar said:

    @locallunatic said:

    A note for the non-.Net people: Random.Next(min, max) the minimum is inclusive and the max is exclusive so you don't get a missed case in the switch statement.

    To be clear: this does work. It's just fucking stupid.

    Yeah, I just figured I'd stop someone from pointing out supposed different behaviors between the two.  If they didn't know about the max being exclusive it looks like the top one will sometimes get 6 and with no case statement for it not set the image where as yours (with the same misunderstanding of Next) looks like it does set in that case.  Just trying to clarify for people as the mismatch on inclusivity/exclusivity has tripped me up before (don't care which you do, but make the two match).


  • Trolleybus Mechanic

    @mikeTheLiar said:

    string backgroundImage = "divImage" + new Random().Next(1, 6).ToString();
     

    Way to not use String.Format()!  I guess you just like wasting webserver CPU cycles on meaningless concatenation.

    In all seriousness, though-- why not do divImage.ImageUrl = "~/Images/Background" + RandomAsFuckNumber + ".png"? What's with messing with the attributes in the codebehind?



  • @Lorne Kates said:

    @mikeTheLiar said:

    string backgroundImage = "divImage" + new Random().Next(1, 6).ToString();
     

    Way to not use String.Format()!  I guess you just like wasting webserver CPU cycles on meaningless concatenation.

    In all seriousness, though-- why not do divImage.ImageUrl = "~/Images/Background" + RandomAsFuckNumber + ".png"? What's with messing with the attributes in the codebehind?


    Good call on String.Format(). Blinded by stupidity. And your solution is probably what I'm going to end up doing, I just couldn't stand looking at that fucking switch-case. This hasn't even been committed yet, I always review what I did before I commit. If I can, I try to sleep on it.



  • @mikeTheLiar said:

    This hasn't even been committed yet, I always review what I did before I commit. If I can, I try to sleep on it.

    Man, you gotta sleep on a change like that? How fucked is your job?



  • @mikeTheLiar said:

    And your solution is probably what I'm going to end up doing,

    Actually, scratch that. I'll probably just in-line it.



  • I would have committed it as soon I made a change that "major". You can always commit a second time later if you further optimize. I'm guessing you're using SVN where commits are scary and rebasing doesn't exist to have that mentality about waiting on commits. Lots of little commits merge much more easily than a few giant multi-file-spanning ones.



  • @morbiuswilters said:

    @mikeTheLiar said:
    This hasn't even been committed yet, I always review what I did before I commit. If I can, I try to sleep on it.

    Man, you gotta sleep on a change like that? How fucked is your job?


    My job is pretty fucked. But good news is that it looks like it might get better (look for more on this in the future). And I don't have to sleep on a change that simple, (see This makes me sad and angry). That being said, this entire code base is a house of cards and toothpicks build on top of a swamp of shit. Making anything beyond the simplest change needs to be handled delicately.



  • @Lorne Kates said:

    In all seriousness, though-- why not do divImage.ImageUrl = "~/Images/Background" + RandomAsFuckNumber + ".png"? What's with messing with the attributes in the codebehind?
    Are you seriously suggesting hardcoding the paths to the images in the code (in god knows how many different locations) is better than keeping them in the CSS and handling the assignment via style classes? Or are you just trolling? It's sometimes hard to tell in this forum...



  • @Anonymouse said:

    @Lorne Kates said:

    In all seriousness, though-- why not do divImage.ImageUrl = "~/Images/Background" +
    RandomAsFuckNumber + ".png"? What's with messing with the attributes in
    the codebehind?
    Are you seriously suggesting hardcoding the paths to the images in the code (in god knows how many different locations) is better than keeping them in the CSS and handling the assignment via style classes? Or are you just trolling? It's sometimes hard to tell in this forum...

    I also note that the original code seemed to clear all attributes from the div. That doesn't seem very smart.


  • Trolleybus Mechanic

    @Anonymouse said:

    Are you seriously suggesting hardcoding the paths to the images in the code (in god knows how many different locations) is better than keeping them in the CSS and handling the assignment via style classes?
     

    1) I would hope this would be a Masterpage, and only a Masterpage. It's a forlorn hope. (get it?)

    2) How is hardcoding them in the css NOT hardcoding the images?  Then all you're doing is hardcoding the css classes to hardcoded image paths. Much easier to decipher.

    3) If you use CSS, you have to send every single class declaration on every single page. Hopefully the css is in its own .css file and is aggressively cached. More likely it's inline.

    4) You can make this more dynamic, I suppose. Dump all the image paths into the database. Select a random one. Done. (Maybe cache the dataset somewhere so you don't nutpunch the database each page load).

    5) You get to use ~/images. I can't wait to see how explosively the OP's coworker's solution breaks the minute they introduce a subfolder. In fact, I can pretty much predict with 104.7% accuracy (+/- 0%) exactly how it will go.

    Mike: I just created a subfolder, and none of the images are working.  Your css uses relative paths. There's obviously no /mikeTheTurthTeller/images/backboob.jpg.  Please make them work properly with subfolders.

    Coworker: I HAVE COMPLETED THIS TASK! I SPEAK LIKE AN INTERNET DOG!

    Mike: Wait, why are there twice as many css class declarations now?

    @CSS said:

    .BackgroundImage1 { background-image: url(images/background1.jpg; }
    ...
    .BackgroundImage1Sub {background-image: url(../images/background1.jpg; }


    Coworker: I AM A GOOD CODER RUB MY TUMMY!

    Mike: FFS! Rejected. Bad. Fix this so it will work with subpaths properly. We might have, like, five subfolders on this project! Okay, max six, but still, it's arbitrary

    Coworker: I NOD WHEN YOU SAY ARBITRARY SO YOU THINK I KNOW IT I DO THAT!

    @CSS_new said:

    .BackgroundImage1 { background-image: url(images/background1.jpg; }
    ...
    .BackgroundImage1Sub {background-image: url(../images/background1.jpg; }
    ...
    .BackgroundImage1SubSub {background-image: url(../../images/background1.jpg; }
    ...
    .BackgroundImage1SubSubSbu {background-image: url(../../../images/background1.jpg; }
    ...
    .BackgroundImage1SubSubSbuSub {background-image: url(../../../../images/background1.jpg; }
    ...
    .BackgroundImage1SubSubSbuSubFive {background-image: url(../../../../../images/background1.jpg; }
    ...
    .BackgroundImage1SubSubSbuSubFiveMax {background-image: url(../../../../../../images/background1.jpg; }


    Coworker: I GET BEEF COOKIE NOW!

     

     


  • Considered Harmful

    Relative paths in CSS are resolved relative to the path of the CSS file, so the described problem doesn't actually exist.



  • @Lorne Kates said:

    Best goddamned thing I've read in a long time.

    You just made my day. Replace "co-worker" with "my predecessor and current manager" and it's a pretty accurate description of my job.



  • @Lorne Kates said:

    2) How is hardcoding them in the css NOT hardcoding the images?  Then all you're doing is hardcoding the css classes to hardcoded image paths.
    CSS files - if used sanely (and I admit that is saying much) can theoretically be changed by a simple drop-in replacement, if you for example want to re-skin the site to a new design. Or you could even serve up completely different designs dynamically for the same site (like different looks for different customers) with little effort by just delivering the exact same HTML, just with different CSS.

    Deploying executable code usually requires quite a lot more hoops for you to jump through (hopefully you have a decent build/deployment environment set up, that can take care of much of the hassle).

    @Lorne Kates said:

    3) If you use CSS, you have to send every single class declaration on every single page.
    Well, duh. Your point being?

    @Lorne Kates said:

    Hopefully the css is in its own .css file and is aggressively cached. More likely it's inline.
    As I said, I am assuming a sane use of CSS, since we already saw in the OP enough of how not to do it.



  • Besides, CSS with WebForms is the wrong way to do it. You should be using strongly typed System.Drawing.Image objects.

    protected void Page_Load(object sender, EventArgs e)
    {
    	Int32 n = RandomNumber(1, 6);
    
    Image image <span style='color:#808030; '>=</span> <span style='color:#800000; font-weight:bold; '>null</span><span style='color:#800080; '>;</span>
    
    <span style='color:#800000; font-weight:bold; '>switch</span> <span style='color:#808030; '>(</span>n<span style='color:#808030; '>)</span>
    <span style='color:#800080; '>{</span>
    	<span style='color:#800000; font-weight:bold; '>case</span> <span style='color:#008c00; '>1</span><span style='color:#808030; '>:</span>
    		<span style='color:#696969; '>//execute code block 1</span>
    		strClassName <span style='color:#808030; '>=</span> Properties<span style='color:#808030; '>.</span>Resources<span style='color:#808030; '>.</span>divImage1<span style='color:#800080; '>;</span>
    		<span style='color:#800000; font-weight:bold; '>break</span><span style='color:#800080; '>;</span>
    	<span style='color:#800000; font-weight:bold; '>case</span> <span style='color:#008c00; '>2</span><span style='color:#808030; '>:</span>
    		<span style='color:#696969; '>//execute code block 2</span>
    		strClassName <span style='color:#808030; '>=</span> Properties<span style='color:#808030; '>.</span>Resources<span style='color:#808030; '>.</span>divImage2<span style='color:#800080; '>;</span>
    		<span style='color:#800000; font-weight:bold; '>break</span><span style='color:#800080; '>;</span>
    	<span style='color:#800000; font-weight:bold; '>case</span> <span style='color:#008c00; '>3</span><span style='color:#808030; '>:</span>
    		<span style='color:#696969; '>//execute code block 2</span>
    		strClassName <span style='color:#808030; '>=</span> Properties<span style='color:#808030; '>.</span>Resources<span style='color:#808030; '>.</span>divImage3<span style='color:#800080; '>;</span>
    		<span style='color:#800000; font-weight:bold; '>break</span><span style='color:#800080; '>;</span>
    	<span style='color:#800000; font-weight:bold; '>case</span> <span style='color:#008c00; '>4</span><span style='color:#808030; '>:</span>
    		<span style='color:#696969; '>//execute code block 2</span>
    		strClassName <span style='color:#808030; '>=</span> Properties<span style='color:#808030; '>.</span>Resources<span style='color:#808030; '>.</span>divImage4<span style='color:#800080; '>;</span>
    		<span style='color:#800000; font-weight:bold; '>break</span><span style='color:#800080; '>;</span>
    	<span style='color:#800000; font-weight:bold; '>case</span> <span style='color:#008c00; '>5</span><span style='color:#808030; '>:</span>
    		<span style='color:#696969; '>//execute code block 2</span>
    		strClassName <span style='color:#808030; '>=</span> Properties<span style='color:#808030; '>.</span>Resources<span style='color:#808030; '>.</span>divImage5<span style='color:#800080; '>;</span>
    		<span style='color:#800000; font-weight:bold; '>break</span><span style='color:#800080; '>;</span>
    <span style='color:#800080; '>}</span>
    
    
    <span style='color:#800000; font-weight:bold; '>this</span><span style='color:#808030; '>.</span>divImage<span style='color:#808030; '>.</span>BackgroundImage <span style='color:#808030; '>=</span> image<span style='color:#800080; '>;</span>
    

    }

    Yes, I know nothing about WebForms other then it's supposed to be WinForms but rendering to HTML instead of GDI+.


  • Trolleybus Mechanic

    @Anonymouse said:

    CSS files - if used sanely (and I admit that is saying much) can theoretically be changed by a simple drop-in replacement
     

    Agreed, but then I wouldn't be sending out different class names. I'd give the element a single class, like "ui-state-hover", and change the css file as needed to alter the appearance.

    @Anonymouse said:

    @Lorne Kates said:
    3) If you use CSS, you have to send every single class declaration on every single page.
    Well, duh. Your point being?

    Bandwidth.  If I set the image path server-side, I send bytes equal to exactly 1 URL.  If I set the css class server side, I have to send bytes equal to n URLs (where n is the number of possible images, I guess 6 in the OP's case).  I also have to send bytes for the css file.

    Changing the page image based on class name is stupid, because I'm sending the 1 image path the client needs, and n-1 paths they don't.

    Again IN THEORY the css could be in its own heavily cached file, and would pay for itself after so many requests. But it's still stupid.

    @Anonymouse said:

    As I said, I am assuming a sane use of CSS, since we already saw in the OP enough of how not to do it.

    There's no such thing as a sane use of CSS.  Just sometimes its level of insanity is less than how crazy it is doing it without CSS.

    The OP's case is not one of them.

     

     


  • Trolleybus Mechanic

    @joe.edwards said:

    Relative paths in CSS are resolved relative to the path of the CSS file, so the described problem doesn't actually exist.
     

    1) Assuming a sane use of css

    2) Unless you're using Internet Explorer (for certain values of timepod)



  • @Lorne Kates said:

    I guess 6 in the OP's case

    CALLED IT!



  • @Lorne Kates said:

    @mikeTheLiar said:

    string backgroundImage = "divImage" + new Random().Next(1, 6).ToString();
     

    Way to not use String.Format()!  I guess you just like wasting webserver CPU cycles on meaningless concatenation.

    In all seriousness, though-- why not do divImage.ImageUrl = "~/Images/Background" + RandomAsFuckNumber + ".png"? What's with messing with the attributes in the codebehind?

     

    http://geekswithblogs.net/BlackRabbitCoder/archive/2010/05/10/c-string-compares-and-concatenations.aspx

    It really doesn't matter unless you are putting massive strings together. Also the oldschool .NET 1.1 reason was because it increased memory usage not CPU time. TBH I expect the newer .NET compilers to do something sensible with it.

     



  • @Lorne Kates said:

    Again IN THEORY the css could be in its own heavily cached file, and would pay for itself after so many requests. But it's still stupid.

    Zwuh? I mean, that's not "in theory", that's just plain, basic Internet-ing 101. True, the first load will probably use a bit more bandwidth to pull in that CSS file, but once it's cached subsequent requests will use less bandwidth.

    @Lorne Kates said:

    There's no such thing as a sane use of CSS. Just sometimes its level of insanity is less than how crazy it is doing it without CSS.

    Buh? Have you ever tried LESS or SASS? They both make CSS more sensible. But overall I'd say CSS is far preferable to doing inline styles (which is still technically CSS--I think you're arguing against external stylesheets and not CSS..)



  • @lucas said:

    It really doesn't matter unless you are putting massive strings together.

    I think he was being facetious, at least slightly. That's something that bugs me, people who are all like "I use String Builders because it's more efficient, durr!" Congratulations, you save four nano-seconds!



  • @locallunatic said:

    @Lorne Kates said:

    I guess 6 in the OP's case

    CALLED IT!

    That is pretty funny.

  • Considered Harmful

    @mikeTheLiar said:

    @lucas said:
    It really doesn't matter unless you are putting massive strings together.

    I think he was being facetious, at least slightly. That's something that bugs me, people who are all like "I use String Builders because it's more efficient, durr!" Congratulations, you save four nano-seconds!

    Bonus points when they're actually slower.


  • Trolleybus Mechanic

    @morbiuswilters said:

    Zwuh? I mean, that's not "in theory", that's just plain, basic Internet-ing 101
     

    WERE YOU THERE?

    @morbiuswilters said:

    @Lorne Kates said:
    There's no such thing as a sane use of CSS. Just sometimes its level of insanity is less than how crazy it is doing it without CSS.

    Buh? Have you ever tried LESS or SASS? They both make CSS more sensible. But overall I'd say CSS is far preferable to doing inline styles (which is still technically CSS--I think you're arguing against external stylesheets and not CSS..)

    .... ew.

    I think I'm arguing against inline stylesheets. Either do something inline, one-off on the element itself, or farm the stylesheet off to it's own file.  Putting a shit-normous stylesheet inline in the page itself (and by that I mean <style type='text/css'>fuckloadofstyle</style>) is the worst of both worlds.

    What I'm arguing against is doing exactly what the OP is doing.

    1) Have a different things a single element can be. In this case, one element can have one of 6 5 background images.

    2) Put the definition of all those million things into a single CSS block, each with their own class name.

    3) On the server-side, decide which of those million 999,999 things your single element will be, and assign it the appropriate client-side class

    4) Provide the client with the definition of every single class, but no way of switching between them.

    It's just as bad as picking a single color for a hyperlink, then sending a style sheet with nothing but

    a.Red {color: red}; a.Blue {color: blue}....

     

     



  • @Lorne Kates said:

    Putting a shit-normous stylesheet inline in the page itself

    Ew, no, don't ever do this unless you're just trying to make a completely self-contained page.



  • @Lorne Kates said:

    @morbiuswilters said:

    Zwuh? I mean, that's not "in theory", that's just plain, basic Internet-ing 101
     

    WERE YOU THERE?

    @morbiuswilters said:

    @Lorne Kates said:
    There's no such thing as a sane use of CSS. Just sometimes its level of insanity is less than how crazy it is doing it without CSS.

    Buh? Have you ever tried LESS or SASS? They both make CSS more sensible. But overall I'd say CSS is far preferable to doing inline styles (which is still technically CSS--I think you're arguing against external stylesheets and not CSS..)

    .... ew.

    I think I'm arguing against inline stylesheets. Either do something inline, one-off on the element itself, or farm the stylesheet off to it's own file.  Putting a shit-normous stylesheet inline in the page itself (and by that I mean <style type='text/css'>fuckloadofstyle</style>) is the worst of both worlds.

    What I'm arguing against is doing exactly what the OP is doing.

    1) Have a different things a single element can be. In this case, one element can have one of 6 5 background images.

    2) Put the definition of all those million things into a single CSS block, each with their own class name.

    3) On the server-side, decide which of those million 999,999 things your single element will be, and assign it the appropriate client-side class

    4) Provide the client with the definition of every single class, but no way of switching between them.

    It's just as bad as picking a single color for a hyperlink, then sending a style sheet with nothing but

    a.Red {color: red}; a.Blue {color: blue}....

     

     

    This is all wrong. The separate stylesheet is a major security risk (using DNS poisoning or some other conveniently not mentioned hacking method someone could hijack the look of the page!). Instead it's better to dynamically generate html with hard-coded style using a SoC templating system. Something simple and convenient such as Paloose, which allows one to quickly design a Wordpress plugin that shows a family tree*.



    • yes this example has it all: XML, XSL, GEDCOM, Wordpress, PHP...


  • @Lorne Kates said:

    If I set the css class server side, I have to send bytes equal to n URLs (where n is the number of possible images, I guess 6 in the OP's case).  I also have to send bytes for the css file.

    Changing the page image based on class name is stupid, because I'm sending the 1 image path the client needs, and n-1 paths they don't.

    Dude, seriously? In what year did your DeLorean drop you off this time? 1985 again? The bandwith requirement for sending 5 URLs instead of 1 worries you? In a file that should be sent over the wire exactly once and then cached, in any half decent implementation?

    I believe that in a typical, medium sized web application, that handful of extra bytes is not going to make much of a difference. That's why I still think the class based solution is the simplest way to accomplish the desired effect that keeps the presentation/design neatly separated from the content and code-behind (that's a good thing, remember?) and the overhead is negligible, IMO. 

    Of course, not all solutions work equally well on all scales, so if there were a lot more (I admit, I'm not sure where I would draw the line. Ten? Uh... probably not. A hundred? Yeah, most likely. A thousand? Oh, absolutely!) different images, each with their respective URLs, that would in fact be a WTF and would mean having to look for a better way. But 5? Just not worth the effort.

     

     


Log in to reply