1
\$\begingroup\$

In this code snippet, I want to be able to return the Amount as it is, unless the CashFlowTypeID is 12, 13, 14 or 15, in which case I want to return Amount, but negated.

How can this code snippet be improved? (Would you consider bitwise operations?)

public decimal GetAmount(decimal Amount, int CashFlowTypeID)
{
    if (new int[] { 12, 13, 14, 15 }.Contains(CashFlowTypeID))
    {
        Amount = Math.Abs(Amount) * -1;
    }
    else
    {
        Amount = Math.Abs(Amount);
    }
    return Amount;
}
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Please, provide review/comment for downvotes! \$\endgroup\$
    – levi
    Mar 13, 2015 at 11:12
  • 3
    \$\begingroup\$ @levi It's not that the code is not understood. It's that the question is made significantly better when a plain-English description of it is included. Moreover, having a plain-English description can sometimes highlight a discepency between the code you've written and the code you think you've written. \$\endgroup\$
    – nhgrif
    Mar 13, 2015 at 11:23
  • 3
    \$\begingroup\$ I don't have a problem understanding the lines at all, I just feel that all code snippets, no matter how simple they are, deserves a bit of explanation to make better quality Code Review questions. If there was a short explanation about what the code does, it would be even easier and quicker to understand your code. If you would In this case, I have more problems understanding why you are doing it than what you are doing. \$\endgroup\$ Mar 13, 2015 at 11:24
  • 2
    \$\begingroup\$ @levi that concerns comments in code. We are asking for background information on your question. And for what it is worth GetAmount is very vague and I have no idea what it is for. GetAmount of what? What's the significance of your magic numbers? \$\endgroup\$
    – Dan
    Mar 13, 2015 at 11:32
  • 3
    \$\begingroup\$ @levi that just repeats what the code does, which is trivial from inspection. Why are you doing that? What is special about those numbers? This is the problem with reviewing out-of-context snippets; how can we possibly give high-quality feedback without understanding the design? It looks like an enumerator would make the code clearer, for example, but without more information it's hard to say. \$\endgroup\$
    – jonrsharpe
    Mar 13, 2015 at 11:36

4 Answers 4

12
\$\begingroup\$

I'm not a C# expert, but I can't imagine unnecessarily creating an array just to use .Contains is very efficient... nor can I imagining even using .Contains is particularly efficient.

I can't imagine anyway in which bitwise operations would be useful here given the current values.


To comment, or not to comment. That is the question.

Or is it.

In the comments, you've linked to this question on StackOverflow to explain your lack of comments: What's the golden code/comment ratio?

Personally, I tend to agree with the point Brad Wilson makes in his answer (not completely, but at least partially), but that doesn't mean everyone is exempt from writing clean, clear, understandable code. Importantly, because one answer with 41 upvotes says "comments should be rare" doesn't mean your code devoid of comments is "doin' it right!".

Before we can graduate to rare comments, we must first be writing self-documenting code. Your code is far from self-documenting.

Here are the following questions I scratch my head over when looking at this method:

  1. Why is a method called GetAmount expecting an amount to be passed in?
  2. After looking at the body, why are we doing any of this? We don't we define amount elsewhere in our code to be an unsigned int and take steps to ensure it's always unsigned? decimal is a signed type... more and more and more headscratching with why this method even exists at all...
  3. What's special about 12, 13, 14, and 15? Why not 1, 2, 3, and 4? Why not any other set of numbers? Why are there four? Could there be more in the future? Could there be less? What's the likelihood of this set of numbers changing?
  4. What are the other possible valid values for cashFlowTypeId? int has a range of -2,147,483,648 to 2,147,483,647. Are all of these values (except 12, 13, 14, and 15) valid values that should return a positive number for amount? Or is this an oversight, and numerous values should be throwing an exception?

Until your code can begin to answer these questions without comments, your code needs comments. I'm not suggesting you add comments. I'm suggesting making your source code capable of answering these questions. That might mean simply creating enums and const variables and finding better names for variables and methods. It might mean adding //comments. It doesn't matter. The point is, your code leaves a maintainer asking a lot of questions and having to do a lot of research before anything is changed, and you need to correct that.


Code written on fewer lines is not more efficient for the compiler OR the maintainer.

Why don't we just use a switch?

public decimal GetAmount(decimal amount, int cashFlowTypeId)
{
    var rtnAmount = Math.Abs(amount);
    switch (cashFlowTypeId) {
        case 12:
        case 13:
        case 14:
        case 15:
            return rtnAmount * -1;
        default:
            return rtnAmount;
    }
}

It's not one line, but is more readable and more efficient.


Magic numbers, pulled from a hat.

Finally, 12, 13, 14, and 15 and magic numbers. These should be named constants so that this method makes more sense to people with less context.


Bitwise operators.

I don't know how much control you have over the cashFlowTypeId, nor do I know how many cashFlowTypeIds there are. However, if we have plenty of control over this aspect, we can make this method both more efficient and shorter with carefully chosen numbers.

Consider that an int is 32-bit in C#, I believe. This means using only individual bits, we can signify 32 different cash flow types. Is this enough? We could change to a long and get 64-bits. Is this enough? Or can we use negative numbers? Negative IDs can signify cash flow types that should return negative, and positive can signify cash flow types that should return positive.

If we use the negative/positive idea and treat 0 as non-negative, we could simply do:

public decimal GetAmount(decimal amount, int cashFlowTypeId)
{
    return Math.Abs(amount) * ((cashFlowTypeId >= 0) ? 1 : -1);
}

It's one line, but still perfectly readable and perfectly efficient.

Alternatively, if we're certain the number of cash flow types is limited, let's actually use a bitwise operator.

But first, to make things easier on ourselves, let's create an enum and define the different IDs.

enum CashFlowType
{
    None = 0 << 0;
    DebitAccountA = 1 << 0;
    DebitAccountB = 1 << 1;
    DebitAccountC = 1 << 2;
    CreditAccountA = 1 << 3;
    CreditAccountB = 1 << 4;
    CreditAccountC = 1 << 5;
}

And we just keep adding to this enum as necessary. If we're going to use bitwise operations however, it's important that every value only takes a single bit (in the correct position) to represent.

We might consider special cases to represent the types that should be negative versus the types that should be positive.

For example:

DebitAccounts = DebitAccountA | DebitAccountB | DebitAccountC
CreditAccounts = CreditAccountA | CreditAccountB | CreditAccountC

So now, let's use some bitwise operators.

public decimal GetAmount(decimal amount, CashFlowType cashFlowType)
{
    // assuming we didn't make a special case for it in our enum:
   CashFlowType creditAccounts = CreditAccountA | CreditAccountB | CreditAccountC;

    return Math.Abs(amount) * ((cashFlowType & creditAccounts) ? -1 : 1);
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ +1 for "It's not one line, but is more readable". The size of someone's code in terms of line rarely matters. What really matters is the the ability for other programmers understand your code without going through mental gymnastics. \$\endgroup\$
    – Dan
    Mar 13, 2015 at 11:18
  • 1
    \$\begingroup\$ "Until your code can begin to answer these questions without comments, your code needs comments." < that is an excellent point well made. \$\endgroup\$
    – jonrsharpe
    Mar 13, 2015 at 15:25
2
\$\begingroup\$

I would remove capital letters from variables (just for convention), not use explicit array typing if you are only using ints and perform operations you perform every time (Math.Abs(amount)) outside of a conditional check.

    public decimal GetAmount(decimal amount, int cashFlowTypeId)
    {
        var absAmount = Math.Abs(amount);

        if (new [] { 12, 13, 14, 15 }.Contains(cashFlowTypeId))
        {
            return absAmount * -1;
        }

        return absAmount;
    }
\$\endgroup\$
3
  • \$\begingroup\$ Answers should contain information on what, and why, it was changed. A simple code sample does not suffice. \$\endgroup\$
    – Dan
    Mar 13, 2015 at 11:07
  • \$\begingroup\$ I was writing that as you downvoted! :P \$\endgroup\$ Mar 13, 2015 at 11:08
  • 2
    \$\begingroup\$ Why did you submit an incomplete answer then? ;-) I'll retract my downvote \$\endgroup\$
    – Dan
    Mar 13, 2015 at 11:09
2
\$\begingroup\$

No need for the else, as if the If statement is true, return value.

public decimal GetAmount(decimal Amount, int CashFlowTypeID)
{
    if (new int[] { 12, 13, 14, 15 }.Contains(CashFlowTypeID))
    {
        return Math.Abs(Amount) * -1;
    }
       return Math.Abs(Amount);
}
\$\endgroup\$
0
-2
\$\begingroup\$

In addition to @enkapsulate's answer, you can even get it onto one line:

public decimal GetAmount(decimal Amount, int CashFlowTypeID)
    {
        return new int[] { 12, 13, 14, 15 }.Contains(CashFlowTypeID) ? Math.Abs(Amount) * -1 : Math.Abs(Amount);
    }
\$\endgroup\$
6
  • 5
    \$\begingroup\$ -1 : code review doesn't stand for one liners but for readable good code. \$\endgroup\$
    – chillworld
    Mar 13, 2015 at 11:09
  • 2
    \$\begingroup\$ You can also get rid of the duplicated Math.Abs call: return Math.Abs(Amount) * (new int[] { 12, 13, 14, 15 }.Contains(CashFlowTypeID) ? -1 : 1); \$\endgroup\$
    – janos
    Mar 13, 2015 at 11:12
  • \$\begingroup\$ @chillworld You'll find that single line if statements are actually more efficient than regular if statements. They may not be as easy to read, however they are certainly faster. \$\endgroup\$
    – Mike Eason
    Mar 13, 2015 at 11:21
  • 1
    \$\begingroup\$ Ternary operators are not faster unless you're writing in a language like Objective-C or Swift which have a special version of the operator that can avoid duplicated method calls. \$\endgroup\$
    – nhgrif
    Mar 13, 2015 at 11:31
  • 1
    \$\begingroup\$ @MikeEason if you are worried about efficient code why not putting the int array as constant. Creating every time a new array costs. \$\endgroup\$
    – chillworld
    Mar 13, 2015 at 11:42

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.