7
\$\begingroup\$

I'm an amateur at programming and have created the following method with which I pass a lowercase string with the idea it is then returned either in it's original lowercase format, Capitalised or UPPERCASE. I'm not sure this is the best way or is truly "random" but does work up to a point but would like pointers on any improvements or different ways to achieve this.

        //==============================================================================
        // Randomise string case
        //==============================================================================
        private string randomCase (string word)
        {
            string changed;
            Random r = new Random(Guid.NewGuid().GetHashCode());
            int n = r.Next(1, 29);

            if (n >= 1 && n <= 9)
            {
                // First letter capatalised
                changed = char.ToUpper(word[0]) + word.Substring(1);
                return changed;
            }
            else if (n >= 10 && n <= 19)
            {
                // Word capatalised
                changed = word.ToUpper();
                return changed;
            }
            else if (n >= 20 && n <= 29)
            {
                // Left as lower case
                return word;
            }

            return null;
        }
\$\endgroup\$
3
\$\begingroup\$

Here are my observations:

  • string randomCase: I would suggest to start your method name with a verb and use CamelCase. For example: RandomizeCase
  • (string word): I've got your intent, but nothing prevents the consumer of your API to put a sentence into your word parameter. In such a general purpose method input can be considered as a fine parameter name.
  • string changed: You don't need this variable at all. You use this as an intermediate container. If you return right after you set it then you can combine these two statements into one, like: return word.ToUpper();
  • Random r / int n: Please try to avoid to use single letter for variable names (exceptions are the loop variables).
  • Random(Guid.NewGuid().GetHashCode()): I've seen so many different ways how to seed a Random. For me the most convenient is the Random((int)DateTime.Now.Ticks). But do you really need to provide a seed at all?
  • r.Next(1, 29): I get it that you want to achieve something like even or fair distribution. This is unnecessary. Your current implementation is hard to maintain and hard to change in the future. Also future reader of your code (even yourself) might ask: Why do we use 29 her as the upper-bound? Why not 316464616?
    • r.Next(1,3) or r.Next() % 3 could be a viable alternative.
  • if ... else if ... return null: Even though you have restricted the range of n you need to provide a return null at the end to make it compile. Even worse this piece of code is never reached. If you want to stick with this approach a better alternative would be to use a single return:
string changed
if (n >= 1 && n <= 9)
{
    changed = char.ToUpper(word[0]) + word.Substring(1);
}
else if (n >= 10 && n <= 19)
{
    changed = word.ToUpper();
}
else if (n >= 20 && n <= 29)
{
    changed = word;
}

return changed;
  • // First letter capatalised: Here you have made a huge assumption: The provided word is in lower case. You can't trust your input. The simplest way to ensure this is to make it lowercase by yourself:
if(string.IsNullOrEmpty(word))
   return string.Empty; //Or throw ArgumentException if you wish

string loweredWord = word.ToLower();
  • The same assumption is present in each of your if-else branches.

UPDATE: Random

It is not crystal clear for me why do you need this method at all, but based on the provided comment I would like highly two things regarding random:

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thanks for the very useful feedback! Very insightful and appreciated. I'm building a random password generator so the string being passed will always be in lower case as all the words in my dictionary are stored in lowercase. The reason I went with Random(Guid.NewGuid().GetHashCode()) was that when reading some online resources about being truly random, comments suggested that was the best approach. I've taken the other suggestions on board and will be sure to implement your suggestions. Thanks again! \$\endgroup\$
    – Rawns
    Jan 20 at 20:10
  • 1
    \$\begingroup\$ @Rawns I've extend my answer with some extra information regarding Random. Please check the suggested articles if you have time. \$\endgroup\$ Jan 21 at 7:41
  • \$\begingroup\$ That's great, thanks! The reason for the method is the UI I have created allows the user to choose to generate a 2, 3 or 4 word long password. To try and make it as random as possible, I wanted each word randomly selected from my dictionary to then have the case randomised using this method. \$\endgroup\$
    – Rawns
    Jan 21 at 19:47
  • 1
    \$\begingroup\$ Random.Next() % 3 is slightly biased. Not much, but if you'd use a value higher than 3 it may become problematic. I'd advise Next(3) instead on a secure random implementation indeed which returns 0, 1 or 2. \$\endgroup\$ Jan 23 at 23:00
3
\$\begingroup\$
  • This entire method should be static, as it clearly does not require an object
  • new Random(Guid.NewGuid().GetHashCode()) is needlessly complicated. new Random() is initialized based on time by default
  • A simpler range from [0,2] can be used.
  • The changed variable can be entirely replaced by a switch and return structure
private static string Capitalized(string word) {
    return char.ToUpper(word[0]) + word.Substring(1);
}

private static string RandomCase(string word)
{
    int choice = new Random().Next(3);
    switch(choice) {
        case 0: return Capitalized(word);
        case 1: return word.ToUpper();
        case 2: default: return word;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Usually the parentheses are used for exclusive and the square brackets for inclusive, so that would be [0, 2). \$\endgroup\$ Jan 23 at 23:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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