Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

The code below gets value from a column in an Excel sheet. The values I get are B1, B2, B3, B4...., B100, ...Bn. All I need to do is strip out the char 'B' and convert the numeric string to integer and count how many times a particular integer has repeated.

The snippet below shows the string stripping the char 'B'. However I feel it can be done in better way but I don't know how.

if (range.Text.ToString() == "BayID")
{
    range = null;
    range = readSheet.get_Range(cell, Missing.Value);
    while (range.Text.ToString() != "")
    {
        range = null;
        string A1 = String.Empty, Val = String.Empty;
        char[] value = null;
        range = readSheet.get_Range(cell, Missing.Value);
        A1 = range.Text.ToString();
        value = A1.ToCharArray();
        j++;
        cell = "D" + j;
        for (int i = 1; i < value.Length; i++)
        { 
            Val += value[i]; 
        }
   }
}
share|improve this question
    
I'm confused about the code, I think the end result of it is that it will do nothing. Also, have a look at the Substring() method. – svick Sep 4 '13 at 11:17
    
Its a half baked cookie.. I am still working on it.. before pasting here I displayed a message box which showed the string and I understood that everything is working as expected.. – vin Sep 4 '13 at 11:22
    
@svick if you would paste that as an answer I would happily accept it.. the Substring() method worked greatly for me! :) – vin Sep 4 '13 at 11:33
    
@vin please edit the title of your post to make it more Google-searchable & improve the site's visibility :) – Mat's Mug Sep 5 '13 at 16:02

Don't be afraid of having more variables. I suspect that the value in range has a different contextual meaning when it is used in the first if clause vs the inside of the while loop. If this is the case, make two different variables with names that describe what the contextual meaning is for the specific variable. The type system will help you know the variable is a Range. A name like price or id (it all depends on what you are using it for) will make the code easier to read.


Building on what @Malachi said: You don't need to define your variables at the beginning and assign them a default value.

string A1 = String.Empty, Val = String.Empty;
char[] value = null;
range = readSheet.get_Range(cell, Missing.Value);
A1 = range.Text.ToString();
value = A1.ToCharArray();

//is the same as

range = readSheet.get_Range(cell, Missing.Value);
string A1 = range.Text.ToString();
char[] value = A1.ToCharArray();
share|improve this answer
    
Good point, I went with the assumption that the range Variable is being used for the same thing every time. – Malachi Sep 4 '13 at 15:33

you have this line

range = null;

and this line

range = readSheet.get_Range(cell, Missing.Value);

both inside and outside of your While Statement. this seems Odd to me? from the code that you have given it looks like you can get rid of

range = null; 

both times that it shows up.

your range variable is already created outside of this if statement, I don't think that you should need to set it to null everytime that you want to change it.

that paired with the Substring() Method that @svick mentioned would clean this up a bit I think.

share|improve this answer

strip out the char 'B' and convert the numberic string to integer and count how many times a particular integer has repeated

// the integer, it's count
Dictionary<int,int> integerCounter = new Dictionary<int,int>;
Regex stripB = new Regex(@"\d+");
int capturedNumber;

// may not be correct use of Worksheet properties
foreach (var cell in range) {
    if (Int32.TryParse(stripB.Match(cell.Text), out capturedNumber)) {

        if (!integerCounter.Contains(capturedNumber)) integerCounter.Add(capturedNumber, 1)
        else
            integerCounter[capturedNumber]++;
   }
}
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.