How to optimize this code? How to make it more elegant?(C#)

Question

1.50/5 (4 votes)

See more:

C#

int a;
         int b;
         int c;
         int d;
         if (a > b)
         {
             c = a;
             d = b;
         }
         else
         {
             c = b;
             d = a;
         }

Posted 28-Oct-14 6:33am

Member 11187200

Add a Solution

6 solutions

Solution 3

There's not much to that. At most, all you can strive for is "more readable". What you show requires a lot of scanning up-and-down to spot the differences between the then and else parts. Have a look at the following; just putting the assignments in closer proximity can make the differences more apparent.

C#

if (a > b) { c = a ; d = b ; }
else       { c = b ; d = a ; }

Posted 28-Oct-14 6:44am

PIEBALDconsult

Comments

Maciej Los 28-Oct-14 12:55pm

+5

Sergey Alexandrovich Kryukov 28-Oct-14 13:12pm

Did you see that the original code won't even compile? :-)
—SA

PIEBALDconsult 28-Oct-14 13:16pm

Who cares? It's just a snippet of a larger whole. If he left off the declarations would you complain that the variables are undeclared?

Sergey Alexandrovich Kryukov 28-Oct-14 13:27pm

No. Here is the criterion: you cannot surround this code by other code to make it compile. This is impossible. If the variable declarations a, b, c, d were not shown at all, you would be right.
See the point now?
—SA

PIEBALDconsult 28-Oct-14 13:32pm

I see the point, but I disagree, I just don't think that's important with such a simple snippet of code. I'm not confused by it and I doubt you are either.

Sergey Alexandrovich Kryukov 28-Oct-14 14:35pm

No, we haven't been confused, but OP could.
Please see my comments to Solution 4, where I explain my point in further detail.
—SA

Solution 4

The code can be optimized to the statement:

because that's really all it does.

/ravi

Posted 28-Oct-14 6:50am

Ravi Bhavnani

Comments

Mehdi Gholam 28-Oct-14 13:09pm

:) 5'ed

Sergey Alexandrovich Kryukov 28-Oct-14 13:18pm

This answer would be great it it was true. Unfortunately, it isn't. Please see my comment below.
—SA

Ravi Bhavnani 28-Oct-14 13:36pm

You're right, Sergey - the code doesn't compile in its current state.

But IMHO, initializing a and b (to get the code to compile) would not really change the values of c and d, as they would "change" from zero to zero, since declaring an int initializes it to Int32.Default. The optimized version of the compileable code would (IMHO) be an empty statement.

/ravi

Sergey Alexandrovich Kryukov 28-Oct-14 14:34pm

No. Your wrong assumption is that a and b are zeros. This is not true. In fact, a and b are either initialized or not. There is no a default value (don't mix it up with type members, which would be initialized to 0). So, initialization is a requirement for compilation. But then, one can right int a = 0; or int a = 10; there is absolutely no preference between those cases. So, you have to consider general case, which does modify c and d. Generally, your way of thinking is very good but not in this case.
One possible proper formulation would be void Modify(int a, int b, out int c, out int d). Then, if one can scratch out a, b, c, d declarations and put the code in the implementation of this method, and the code will actually do the modification. Can you see the point now?
—SA

Ravi Bhavnani 28-Oct-14 15:03pm

> Your wrong assumption is that a and b are zeros.
> This is not true.
> In fact, a and b are either initialized or not.
You're correct - thanks for clearing that up.

/ravi

Sergey Alexandrovich Kryukov 28-Oct-14 15:14pm

Thank you for understanding.
—SA

Sergey Alexandrovich Kryukov 28-Oct-14 13:13pm

Did you see that the original code won't even compile? :-)
~~I would call you answer the absolute winner so far if you mentioned that.~~
After the fix is done, your statement would become false: the code will modify the values of local variables. This is only a fragment. So, you could not say it does nothing; it either ) 1) prevents compilation, 2) after a fix, it would modify c and d.
—SA

Solution 6

For the efficiency, I would usually do something similar to original code.

For elegance, you could write:

C#

var c = Math.Max(a, b);
var d = Math.Min(a, b);

Alternatively, the following patern could sometime also be used:

C#

var c = a;
var d = b;

if (c < d)
{
    var temp = c;
    c = d;
    d = temp;
}

In pratice, it does not mattern much for simple cases like that. When code become more complex, some solutions will become more appropriate.

Posted 28-Oct-14 13:56pm

Philippe Mori

Updated 28-Oct-14 14:03pm

v3

Solution 5

c = (a > b) ? a : b;
d = (a > b) ? b : a;

Posted 28-Oct-14 7:13am

Member 11135298

Comments

Sergey Alexandrovich Kryukov 28-Oct-14 13:15pm

Did you see that the original code won't even compile? :-)
—SA

PIEBALDconsult 28-Oct-14 13:19pm

No. Now it's even less readable and performs the same test twice -- ergo efficiency has been decreased.

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

CPallini · Accepted Answer · 2014-10-28T06:42:00

Solution 1

While you probably can't either optimize or make it more elegant, you might fix it (you are using uninitialized variables).

Posted 28-Oct-14 6:42am

CPallini

Comments

Maciej Los 28-Oct-14 12:54pm

+5

CPallini 28-Oct-14 13:00pm

Thank you, Maciej.

Sergey Alexandrovich Kryukov 28-Oct-14 13:11pm

Exactly, a 5.
—SA

CPallini 28-Oct-14 13:14pm

Thank you.

BillWoodruff 28-Oct-14 23:13pm

+5 Yep!

Mehdi Gholam · Accepted Answer · 2014-10-28T06:42:00

Solution 2

Optimization of the code make no sense without context and even with context the code is too simplistic to even bother.

If it works and you understand what it does then leave it.

Posted 28-Oct-14 6:42am

Mehdi Gholam

Comments

Maciej Los 28-Oct-14 12:54pm

+5!

Mehdi Gholam 28-Oct-14 13:08pm

Cheers Maciej!

BillWoodruff 28-Oct-14 23:04pm

+5 for philosophy ! Unfortunately for the OP, the code as-is will not compile.

Mehdi Gholam 29-Oct-14 1:23am

Well I did say "if it works" :)