7
\$\begingroup\$

I am working on this assignment to draw the flag of Liberia for my essentials of computer programming class.

Flag of Liberia

I finished it but had extra time to kill and noticed that a couple similar blocks of code are not very concise and could be shortened with a loop, but right now I can't seem to form the code just right!

Below I have included one of the methods that I would like to shorten:

public static void flagOfLiberia(Graphics g) {
    Expo.setBackground(g,Expo.black);
    for (int d = 1; d <= numDots; d++) {
        int x = Expo.random(0,1000);
        int y = Expo.random(0,650);

        if (x < 300 && y < 300)
            Expo.setColor(g,Expo.darkBlue);
        else if (y < 50)
            Expo.setColor(g,Expo.red);
        else if (y < 100)
            Expo.setColor(g,Expo.white);
        else if (y < 150)
            Expo.setColor(g,Expo.red);
        else if (y < 200)
            Expo.setColor(g,Expo.white);
        else if (y < 250)
            Expo.setColor(g,Expo.red);
        else if (y < 300)
            Expo.setColor(g,Expo.white);
        else if (y < 350)
            Expo.setColor(g,Expo.red);
        else if (y < 400)
            Expo.setColor(g,Expo.white);
        else if (y < 450)
            Expo.setColor(g,Expo.red);
        else if (y < 500)
            Expo.setColor(g,Expo.white);
        else if (y < 550)
            Expo.setColor(g,Expo.red);
        else if (y < 600)
            Expo.setColor(g,Expo.white);
        else if (y < 650)
            Expo.setColor(g,Expo.red);

        drawDot(g,x,y);
        Expo.setColor(g,Expo.white);
        Expo.fillStar(g,150,150,100,5);
    }
    showName(g,"Liberia");
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ I wouldn't hardcode the resolution to be rendered. \$\endgroup\$
    – kasperd
    Commented Apr 22, 2016 at 9:02
  • 1
    \$\begingroup\$ Why do you iterate over a fixed number of dots and do a random () to find the x, y coords? The flag has fixed coords (I guess 650 x 300 by talking a quick look at your code). Wouldn't it be better to do this with 2 nested loops? \$\endgroup\$
    – mkorman
    Commented Apr 22, 2016 at 9:20
  • \$\begingroup\$ @mkorman The program is written like that because it is part of the assignment. The template file was also written like this because when you run it the program will ask for an execution type of: giant, big, small, and tiny dots. This is important because the dots are being rendered randomly, so if the dots are much bigger than it will take less time to render. \$\endgroup\$
    – Smarticles101
    Commented Apr 22, 2016 at 10:15
  • 1
    \$\begingroup\$ Hi, I rolled back your edit. Please do not update the code in your question to incorporate feedback from answers. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Kaz
    Commented Apr 22, 2016 at 10:48

1 Answer 1

Reset to default
7
\$\begingroup\$ 
public static void flagOfLiberia(Graphics g) {
    Expo.setBackground(g,Expo.black);
    for (int d = 1; d <= numDots; d++) {
        int x = Expo.random(0,1000);
        int y = Expo.random(0,650);

        if (x < 300 && y < 300)
            Expo.setColor(g,Expo.darkBlue);
        else if (y % 100 < 50)
            Expo.setColor(g,Expo.red);
        else
            Expo.setColor(g,Expo.white);

        drawDot(g,x,y);    // must be run inside the loop
    }
    Expo.setColor(g,Expo.white);
    Expo.fillStar(g,150,150,100,5);
    showName(g,"Liberia");
}

The above should be at least a little improvement. I used a modulus (%) to check every multiple of 100, seeing if the remainder is less than or greater than 50. If you desire a cutoff at 650, then a single if statement can be added before the modulus to check that y is below the threshold. Also, I placed the star after the for loop in order to execute it only once.

\$\endgroup\$
5
  • \$\begingroup\$ @Thomas ahh yes! I will just draw the star at the end because that doesn't need to be repeated. As for your other comment, the bgcolor needs o be black because the program doesn't render the flag completely and it leaves holes in it. The black makes it easy to see the holes \$\endgroup\$
    – Smarticles101
    Commented Apr 22, 2016 at 10:30
  • 1
    \$\begingroup\$ Ah, ok. I assumed you would not want to see so many holes, but fair enough. \$\endgroup\$
    – Thomas
    Commented Apr 22, 2016 at 10:34
  • 3
    \$\begingroup\$ if...else should always have curly brackets. \$\endgroup\$
    – Boris the Spider
    Commented Apr 22, 2016 at 12:42
  • 1
    \$\begingroup\$ @Boris Why so you say that? Having no brackets, in this situation, saves storage and screen space while maintaining readability. \$\endgroup\$
    – StardustGogeta
    Commented Apr 22, 2016 at 15:08
  • 2
    \$\begingroup\$ Don't take it from me, take it from the great Jon Skeet. Or read about this disaster at Apple. Basically, "saves .. screen space" is never a good reason to do anything. Saving storage maybe - but certainly not in this case. Not having curly brackets hurts maintainability which, in real code, is sacrosanct and should never be harmed, not for any reason. It is widely agreed that curly brackets should always be used. \$\endgroup\$
    – Boris the Spider
    Commented Apr 22, 2016 at 15:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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