5
\$\begingroup\$

I'm working with the following code which adjusts an int based on multiple elements of an integer array. I am wondering if there's a cleaner, easier-to-read construct for this:

int Blue = 0;
int[] Adjustments = new int[24];
// ... populate Adjustments

if (Adjustments[0] == 255)
    Blue = 128;
else if (Adjustments[0] == 0)
    Blue = 0;

if (Adjustments[1] == 255)
    Blue += 64;
else if (Adjustments[1] == 0)
    Blue += 0;

if (Adjustments[2] == 255)
    Blue += 32;
else if (Adjustments[2] == 0)
    Blue += 0;

if (Adjustments[3] == 255)
    Blue += 16;
else if (Adjustments[3] == 0)
    Blue += 0;

if (Adjustments[4] == 255)
    Blue += 8;
else if (Adjustments[4] == 0)
    Blue += 0;

if (Adjustments[5] == 255)
    Blue += 4;
else if (Adjustments[5] == 0)
    Blue += 0;

if (Adjustments[6] == 255)
    Blue += 2;
else if (Adjustments[6] == 0)
    Blue += 0;

if (Adjustments[7] == 255)
    Blue += 1;
else if (Adjustments[7] == 0)
    Blue += 0;

Any suggestions?


Edit

Here is the revised version based on suggestions provided:

Blue = 0;
Green = 0;
Red = 0;
int[] Adjustments = new int[24];
// ... populate Adjustments

int[] AdjustmentStops = new[] { 128, 64, 32, 16, 8, 4, 2, 1 };
for (int i = 0; i < 8; i++)
{
    Blue += Adjustments[i] == 255 ? AdjustmentStops[i] : 0;
    Green += Adjustments[i + 8] == 255 ? AdjustmentStops[i] : 0;
    Red += Adjustments[i + 12] == 255 ? AdjustmentStops[i] : 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You could just write 1 << (7-i) instead of AdjustmentStops[i] and get rid of the array, but it’s up to you to decide whether you find that too unreadable. \$\endgroup\$ – Timwi Apr 5 '11 at 16:45
2
\$\begingroup\$

How about the following?

int Blue = 0;
int[] Adjustments = new int[ 24 ];
int[] BlueAdditions = new int[] { 128, 64, 32, 16, 8, 4, 2, 1 };
// ... populate Adjustments

for ( int i = 0; i < 8; ++i )
{
    if ( Adjustments[ i ] == 255 )
    {
        Blue += BlueAdditions[ i ];
    }
}

Why do you do Blue += 0? That's pretty pointless, so I removed those.

Furthermore, why is Adjustments 24 items long? I'm guessing you use the other items at a later moment, if you don't, you can remove the magic number 8 and change it to Adjustments.Length.

\$\endgroup\$
  • \$\begingroup\$ This code is something I inherited; I suspect at one time the += 0 parts had a different amount and have never been omitted. Your idea of an array holding the adjustment stops is great. \$\endgroup\$ – JYelton Mar 30 '11 at 18:04
  • \$\begingroup\$ Adjustments is 24 elements because there are also red and green values adjusted later, but I only included that which I think got the point across. :) \$\endgroup\$ – JYelton Mar 30 '11 at 18:05
  • \$\begingroup\$ @JYelton: In that case I would opt to encapsulate this code with an adjustments array of 8 long, and run it for red, green and blue separatly. \$\endgroup\$ – Steven Jeuris Mar 30 '11 at 18:10
7
\$\begingroup\$

At the risk of being 'clever':

int Blue = 0;
int[] Adjustments = new int[24];
// ... populate Adjustments 

for (int blueIndex=0; blueIndex<8; blueIndex++)
{
   if (Adjustments[blueIndex] == 255)
   {
      Blue |= (1 << (7 - blueIndex));
   }
}

Since in effect you're setting bits anyway I think using bitwise operations is idiomatic here.

\$\endgroup\$
  • \$\begingroup\$ I was curious about a way that implemented the bit position somehow, this does just that. \$\endgroup\$ – JYelton Mar 30 '11 at 19:42
  • 1
    \$\begingroup\$ but you want bitwise OR dont you? |= \$\endgroup\$ – jon_darkstar Mar 30 '11 at 22:20
  • \$\begingroup\$ @jon_darkstar: You're right :) Edited. \$\endgroup\$ – Michael K Mar 30 '11 at 23:23
  • 1
    \$\begingroup\$ @jon_darkstar - Bitwise AND doesn't have the same meaning as "and" in English. Bitwise AND says, "make the result true if both inputs are true," while in English it means "both statements are true". For example, "He gave me the apples and oranges" means that you received the apples and the oranges while "He gave me the apples AND oranges" means you received nothing because nothing is both an apple and an orange, barring genetic hybrids. \$\endgroup\$ – David Harkness Mar 31 '11 at 1:45
  • 1
    \$\begingroup\$ While I like this solution, I opted for Steven Jeuris' because if (for some reason) the values change it will be easier to modify. Awesome work though. \$\endgroup\$ – JYelton Mar 31 '11 at 21:11
2
\$\begingroup\$
Blue = 0; Red = 0; Green = 0;
for(int i = 0; i < 8; i++)
{
    Blue   |= (Adjustments[i] / 255) * (1 << (i-7));
    Red    |= (Adjustments[i+8] / 255) * (1 << (i-7));
    Green  |= (Adjustments[i+16] / 255) * (1 << (i-7));
}

Or

Blue = 0; Red = 0; Green = 0;
a = 1;
for(int i = 7; i >= 0; i--)
{
    Blue   |= (Adjustments[i] / 255) * a;
    Red    |= (Adjustments[i+8] / 255) * a;
    Green  |= (Adjustments[i+16] / 255) * a;
    a = a << 1;
}
\$\endgroup\$
  • \$\begingroup\$ bet theres a nice way to do this with slices and reduce (or is it aggregate in c#/LINQ)? ill poke around a little and see if i can figure it out \$\endgroup\$ – jon_darkstar Mar 31 '11 at 2:24
-1
\$\begingroup\$

Although perhaps less efficient, any time I see an array being used as a half-assed object, I'm inclined to make an object.

struct ColorAdjustment {
    public int Red;
    public int Blue;
    public int Green; 
}

Then make an array of 8 of those.

But since you're only checking two values (255 or 0) a set of bools would be even better. But since you're just doing bit math, @jon_darkstar's answer is even better still because it just does that math directly.

But best of all would probably be to use a BitArray or to do bit boolean logic on a 24-bit value (or an Int32, if you don't mind wasted space) to indicate the state of adjustments, as:

Int24 adjustments = 0xBEAF;
for(int i = 0; i < 8; i++) {
    if((adjustments && (1 << i)) != 0)
        ...do blue stuff...
    if((adjustments && (1 << (i + 8))) != 0)
        ...do red stuff...
...

Edit: Hah, and I just realized I ran the full gamut from clear, object based code to fiddly bit-based code.

\$\endgroup\$

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.