current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

I have the following method which extracts a decimal value from a string.

using System;
using System.Linq;
using System.Text;

namespace Foo.Bar.Common.Converters
{
    public static class DecimalConverter
    {
        public static Decimal ExtractDecimalFromString(string str)
        {
            var sb = new StringBuilder();
            foreach (var c in str.Where(c => c == '.' || Char.IsDigit(c)))
            {
                sb.Append(c);
            }
            return Convert.ToDecimal(sb.ToString());
        }
    }
}

Is this the most effecticve way to do it?

share|improve this question
    
If you have not a decimal point but a decimal comma, you get not the desired result. Did you make other tests, e.g. how it performs with the Remove method? – Thomas Krojer Mar 26 '15 at 12:29
    
I've not tested it extensively, just though i'll pop it up here. The comma- point is a non issue because the data i run this on will always have a point. – Stingervz Mar 26 '15 at 12:31
2  
Any negative values? Decimals can be negative, right? And your code will do odd things with the input "1 day 3 people bought 5 beers for 8 dollars". – rolfl Mar 26 '15 at 12:45
2  
@Stingervz: please provide some samples. I´m thinking about an break condition for your Loop – Thomas Krojer Mar 26 '15 at 13:18
    
@Stingervz please explain the problem you're solving with this method. Especially, the expected type of input! Some samples are necessary. – ANeves Mar 26 '15 at 14:00
up vote 5 down vote accepted

Regular expressions can be a good way of processing text input in situations like this. Your current process finds every digit in the input string, concatenates it together, and then parses the result.

I doubt that this is a good solution because it makes examples like: "There are 10 people arriving at 9pm" parse as 109. Is that what you want?

Even then, Regular expressions are probably a good solution:

str = Regex.Replace(str, @"[.\D+]", "");

In str, replace all non-digits with "" (i.e. remove any non-digits).

Your code will still fail with an exception for input like "Hello", which has no digits, because the ToDecimal call will fail. Values with multiple . characters will also be interesting...

Additionally, your code does not support negative input values. This can be a trick to accomplish, but, again, using regular expressions, it is not horrendous.

Finally, I feel you should only be parsing the first set of decimal-like digis in the input, not all of them.

Putting this all together, I would have:

  • use regex
  • parse the first set of digits
  • accept negative inputs

And use the code:

    public static Decimal ExtractDecimalFromString(string str)
    {

        Regex digits = new Regex(@"^\D*?((-?(\d+(\.\d+)?))|(-?\.\d+)).*");
        Match mx = digits.Match(str);
        //Console.WriteLine("Input {0} - Digits {1} {2}", str, mx.Success, mx.Groups);

        return mx.Success ? Convert.ToDecimal(mx.Groups[1].Value) : 0;
    }

I have put this, with some test cases, in this Ideone here;

share|improve this answer
up vote 1 down vote

Assuming you only want the first decimal embedded in the string and that your data is strongly typed and formatted, you should be able to short cut the loop by stopping after you extract the number. Something like this:

public static Decimal ExtractDecimalFromString(string str)
{
    var sb = new StringBuilder();
    foreach (var c in str.SkipWhile(c => c != '.' && !Char.IsDigit(c)))
    {
        if(c != '.' && !Char.IsDigit(c))
        {
            return Convert.ToDecimal(sb.ToString());
        }
        sb.Append(c);
    }
    return Convert.ToDecimal(sb.ToString());   
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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