0
\$\begingroup\$

My task was write a method that has input parameter - string and this method must return a collection of symbols which are most frequently , for example:

  • input: ааа bbb с
  • ouput: а, b
  • input: ааа bb с

  • ouput: a

  • а, b, с
    • а b с

Here is my bad code:

 static void test(string s)
        {
            int[] cc = new int[255];
            char c;

            for (c = (char)0; c < 255; c++)
            {
                cc[c] = 0;
            }

            for (int i = 0; i < s.Length; i++)
            {
                cc[s[i]]++;
            }
            int cntMax = 0;


            for ( c =(char) 0; c < 255; c++)
            {
             if(cc[c] > cntMax)
                {
                    cntMax = cc[c];
                }
            }
            string L = "";
            if(cntMax >0 )
            {
                for ( c = (char)0; c < 255; c++)
                {
                    if(cc[c] == cntMax)
                    {
                        if(c > (char) 32)
                        {
                            L = L + c + " ";
                            Console.WriteLine(L);
                        }
                    }



                }
            }
        }

So , I don't sure if understand , I must return collection of symbols , how to do it? How can I change my code?? And also I have a question about input: if I input a b c , then my program count spaces instread of symbols , how to fix it? Sorry for my very bad code

\$\endgroup\$

closed as off-topic by t3chb0t, paparazzo, Vogel612, Toby Speight, Mathias Ettinger Aug 22 '17 at 8:51

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – t3chb0t, Vogel612, Toby Speight, Mathias Ettinger
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I must return collection of symbols , how to do it? This part of your question is off-topic because Code Review isn't about writing code for you. Another part about if I input a b c , then my program count spaces instread of symbols , how to fix it? is off-topic either because your code seems to be broken. Code Review requires you to provide working code. \$\endgroup\$ – t3chb0t Aug 21 '17 at 19:43
  • \$\begingroup\$ @t3chb0t okay I provide working code , I asked how can I refactor it with collection that's all \$\endgroup\$ – M-Misha-M Aug 21 '17 at 19:50
  • \$\begingroup\$ Not getting this for (c = (char)0; c < 255; c++) \$\endgroup\$ – paparazzo Aug 21 '17 at 20:09
  • \$\begingroup\$ At least format better before posting \$\endgroup\$ – paparazzo Aug 21 '17 at 20:12
  • \$\begingroup\$ As an aside, I would argue that the most frequent characters in the string "a, b, c" are "," and " ". Not "a", "b" and "c". \$\endgroup\$ – gamesmad Aug 23 '17 at 9:14
0
\$\begingroup\$

You could filter (depending if you are useing ascii or utf) with some if statements like:

if((charactervar!=32)&(characterval!=44)){
    //filter ascii character for blank/space 32 and comma 44
}

...

static void test(string s)
    {
        int[] cc = new int[255];
        char c;

        for (c = (char)0; c < 255; c++)
        {
            cc[c] = 0;
        }

        for (int i = 0; i < s.Length; i++)
        {
           if((s[i]!=(char)32)&(s[i]!=(char)44)){ //if you use a different encoding than ascii please replace the numbers with the appropriate numbers from the character table
              cc[s[i]]++;
           }
        }
        int cntMax = 0;


        for ( c = (char)0; c < 255; c++)
        {
         if(cc[c] > cntMax)
            {
                cntMax = cc[c];
            }
        }
        string L = "";
        if(cntMax >0 )
        {
            for ( c =(char) 0; c < 255; c++)
            {
                if(cc[c] == cntMax)
                {
                    if(c > 32) //and here
                    {
                        L = L + c + " ";
                    }
                }
            }
        Console.Write(L);
    }

You could also consider the use of regular expressions like here: https://stackoverflow.com/questions/14565934/regular-expression-to-remove-all-non-printable-characters (I know it's java but the concept should be the same)

And I'm not an expert to ask for "pretty" code as I only focus to make it work :). But you could save some lines where you initialize your int array cc. I think there is a way to write that shorter.

\$\endgroup\$
  • \$\begingroup\$ it wrote collection of symbols in task , and u wrote me " Not getting this for (c = (char)0; c < 255; c++) " how to correctly do it?? \$\endgroup\$ – M-Misha-M Aug 21 '17 at 20:19
  • \$\begingroup\$ That was not his comment and you did not answer the question. Focus. \$\endgroup\$ – paparazzo Aug 21 '17 at 20:20
  • \$\begingroup\$ Then at the end he has if(c > (char) 32) \$\endgroup\$ – paparazzo Aug 21 '17 at 20:24
  • \$\begingroup\$ So you want to exclude all things like "space" or "newline". Sorry if I missunderstood your question. \$\endgroup\$ – Beny Benz Aug 21 '17 at 20:26
  • \$\begingroup\$ @BenyBenz yes , my code workd for 1 and 2 cases , but you can see at 3 case and here it is my problem , how to ignore it , also u can see there coma \$\endgroup\$ – M-Misha-M Aug 21 '17 at 20:28
1
\$\begingroup\$

Text and data structures

C# uses UTF-16 internally, so a char is actually 2 bytes long (and even then not all Unicode characters can be stored in a char). An array of 256 (a single byte can have 256, not 255, different values) is not sufficient. A Dictionary<char, int> would be more suitable here. For example, the following code is not limited to Latin characters, so feeding it some Cyrillic should work just fine:

var charCounts = new Dictionary<char, int>();
foreach (char c in s)
{
    if (!charCounts.ContainsKey(c))
        charCounts[c] = 1;
    else
        charCounts[c]++;
}

You said that you want to ignore space characters, but your code also ignores some control characters (but not all). You may want to use char.GetUnicodeCategory to determine which characters should be counted (there are several categories: various kinds of letters, digits, control characters and so on).

As for returning multiple characters, that's what arrays and lists are for. Instead of appending characters to a string, add them to a list and return that list when you're finished.

Other comments

  • Don't use 'magic values' (such as that 255 that's appearing several times). When you iterate an array, use its Length property: for (int i = 0; i < cc.Length; i++).
  • Using a char to index an array makes your code a little confusing to read. I don't see any advantages over using a 'standard' int.
  • I would also not recommend reusing an indexing variable (c): it's easy to introduce bugs by forgetting to 'reset' its value.
  • You don't need to initialize an array of ints with 0's: C# already does that for you (0 is the default value for int).
  • Try using meaningful variable names: text or input instead of s, charCounts or even characterCounts instead of cc, and so on. It'll make your code easier to understand, and that's a good thing when you have to revisit code a few months later.
  • The easiest way to get the maximum value from a collection is to use Linq's Max method: cc.Max(). There are also several other Linq methods that could make this method easier to write, such as GroupBy, Where and ToDictionary.
\$\endgroup\$
  • \$\begingroup\$ Okay , let's move next , I understand about Length in loop , explain please about using int instread of (char) that is present in my "for" loop \$\endgroup\$ – M-Misha-M Aug 21 '17 at 22:52
  • \$\begingroup\$ about dictionary , is it best way stackoverflow.com/questions/40799440/… (code of topic starter) ? \$\endgroup\$ – M-Misha-M Aug 21 '17 at 22:58
  • \$\begingroup\$ @M-Misha-M: the accepted answer to that question shows some improvements over the topic starter, but yeah, that's the general idea. As for what's 'best', that very much depends on requirements, available time, and so on. As for using int instead of char, it's about intent. When you want to look up the count of a certain character, it makes sense to cast a char to an int (index). But if you need to loop over the array, using a char as counter variable only makes things more confusing. \$\endgroup\$ – Pieter Witvoet Aug 21 '17 at 23:09
  • \$\begingroup\$ can u show me how to rewrite code without char?? How my loops looks like \$\endgroup\$ – M-Misha-M Aug 21 '17 at 23:11
  • \$\begingroup\$ I already did: for (int i = 0; i < cc.Length; i++) instead of for ( c =(char) 0; c < 255; c++). \$\endgroup\$ – Pieter Witvoet Aug 21 '17 at 23:12

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