15
\$\begingroup\$

The following is my first successful running C# program.

The lessons that I personally learnt through this include:

  • The value of asking for help after having tried several times
  • The advantages of being adaptable about one's methods of reaching an end
  • The (a?) proper placement of functions in relation to a main program
  • How to pass information into and out of such functions
  • How to use a loop to leverage code (doing the same stuff over and over)
  • How to collect character input from the User & I'm sure there's more...

In any case this code appears to work. I know that it doesn't hold a candle to much of the stuff you guys put up but on the bright side its short and kind of readable (both aims of mine).

Could some much brighter spark than me kindly shine a light on the stuff that could have been done better (or simply provide some advice or suggested direction)? :c)

using System;
namespace CapsChecker
{
    class Program
    {
        static void Main(string[] args)
        {
                //Set up an infinite loop for program to run within 
            while (true)
            {
                    //Collection of user input
                Console.Write("Press a key: ");
                char input = (Console.ReadKey().KeyChar);
                    //Check if Capslock is on when key entered. Combined with 
                    //the shift button this can result in CapsCheck returning
                    //true even if key entered is lower case.
                if (CapsCheck() == true)
                {
                    CapsNotify(true);   
                }
                else
                {
                    CapsNotify(false);
                }
                Console.WriteLine();

                switch (input) //Abandoned '.Key' enum
                {
                    case 'Q':
                        { break; } 
                    default:
                        { continue; }
                }
                break; //Afterthoght as it wasn't breaking on capital Q.
            }
            Console.WriteLine("Program ending. Press a key.");
            Console.ReadKey();
        }

        static bool CapsCheck()
        {
                // Check if Capslock is Active
            if (Console.CapsLock == true)
            {
                return true;
            }
            else
            {
                return false;
            }
        }

        static void CapsNotify(bool caps)
        {
                // Notify whether Capslock is Active
            if (caps == true)
            {
                Console.WriteLine(" Capslock is Active.");
            }
            else
            {
                Console.WriteLine(" Capslock is Inactive.");
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Unfortunately it doesn't seem to work on Linux with X Windows / Mir. I don't really know how one would check for caps lock being turned on in a PTY. \$\endgroup\$ – cat May 14 '16 at 13:04
  • 2
    \$\begingroup\$ Sorry that it didn't work for you cat. If it helps I'm using Visual Studio 2015 (community) on a Windows 7 machine (I really don't like prompts telling me to upgrade to 10 ^_~). \$\endgroup\$ – Avestron May 14 '16 at 15:05
23
\$\begingroup\$

if true return true else return false antipattern

Unecessary if statements when dealing with booleans is a very common beginner pitfall.

    static bool CapsCheck()
    {
            // Check if Capslock is Active
        if (Console.CapsLock == true)
        {
            return true;
        }
        else
        {
            return false;
        }
    }

Is equivalent to:

   static bool CapsCheck()
   {
       return Console.CapsLock;
   }

As a further simplification, you may use Console.CapsLock directly and avoid this function.

Another boolean simplification is

            if (CapsCheck() == true)
            {
                CapsNotify(true);   
            }
            else
            {
                CapsNotify(false);
            }

To:

 CapsNotify( CapsCheck() );

Or:

CapsNotify( Console.CapsLock );

Ternary simplification

In CapsNotify it is unecessary to use a full if else, a ternary is enough and simpler:

 Console.Writeline("CapsLock is" + (caps ? "Active" : "Inactive"));
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very kindly Caridorc! I just tested your modification suggestions and it works perfectly. Not that I doubted you - I just had to try the shiny new tool in my c# toolbox. :c) \$\endgroup\$ – Avestron May 14 '16 at 9:47
  • \$\begingroup\$ Ternary operators are often frowned upon in commercial environments for the sake of clarity of code. \$\endgroup\$ – Killroy May 14 '16 at 10:09
  • 4
    \$\begingroup\$ @Killroy Ternaries are simpler than if / else, if / else allows statements too, ternary only expressions. How can a simpler tool be said to hurt clarity? \$\endgroup\$ – Caridorc May 14 '16 at 10:11
  • \$\begingroup\$ I've found that C programmers are adverse to the ternary for some (what I'm sure is a very good in that environment) reason. In the managed world of .Net, the ternary is good for simple assignments, but if it begins to become complicated, it's better to switch to the more verbose way. We like ternaries on my team, but disallow any nested ternaries from in our code base. \$\endgroup\$ – RubberDuck May 14 '16 at 12:45
  • 1
    \$\begingroup\$ Console.WriteLine($"CapsLock is {caps? "Active" : "Inactive"}"); is arguably more understandable. \$\endgroup\$ – Benjamin Gruenbaum May 15 '16 at 7:21
10
\$\begingroup\$

Instead of using a forever loop with a break, which as you found out, gets complicated when there's a switch involved, evaluate your end condition inside of your while.

So instead of

while(true)
{
    //...

     switch (input) //Abandoned '.Key' enum
     {
        case 'Q':
           { break; } 
        default:
            { continue; }
      }
      break; //Afterthoght as it wasn't breaking on capital Q.
        }

You can just do this

char input = default(char);

do
{

    //...

} while (input != 'Q');

Because you always want your main loop to execute at least once.

See the msdn doc for do...while loops for more information.

\$\endgroup\$
  • \$\begingroup\$ A very good point RubberDuck. :c) I admit that I ran into a little trouble with input being defined from within the loop (and not being retained for the condition). Do you have any suggestion as how to easily skirt this problem? \$\endgroup\$ – Avestron May 14 '16 at 15:00
  • \$\begingroup\$ I'm betting you tried while(input != 'Q') first, right? Try the do...while. I'm pretty confident that it'll solve your problem @Avestron \$\endgroup\$ – RubberDuck May 14 '16 at 15:03
  • \$\begingroup\$ Actually I commented out the 'while...' code at the top, adding a 'do' line in its place. Commented out the Switch statement. Finally added 'while(input != 'Q')' after the close curly bracket after the second commented out break statement... :c) \$\endgroup\$ – Avestron May 14 '16 at 15:08
  • 1
    \$\begingroup\$ @Avestron Yeah, the do {} while() loop cannot test a variable that was defined inside its body (the {}). You can either declare it before the do, or use your previous code, but have a bool value that is set to true if the key is Q, and do the break outside the switch. \$\endgroup\$ – Kroltan May 14 '16 at 15:12
  • 1
    \$\begingroup\$ Back. :c) Placing: char input; before the beginning of the loop (and removing the 'char' before 'input' during user input phase) did the trick :c) Still works well and its neater! Thank you both. \$\endgroup\$ – Avestron May 14 '16 at 19:58
0
\$\begingroup\$

Thank you kindly for all the feedback received. I have learnt a lot from the experience. :c)

Since it would be inappropriate to edit my original post I shall instead post the complete revised code as a wiki answer. It incorporates much of the suggestions received and I do believe it to be a big improvement upon the original.

The code is much shorter if more heavily-commented.

using system;
namespace CapsChecker
{
    class Program
    {
        static void Main(string[] args)
        {
                //input declared before primary program loop 
                //to permit for conditional purposes
            char input;
            do
            {
                    //Collection of User Input
                Console.Write("Press a key: ");
                input = (Console.ReadKey().KeyChar);
                    //Check if Capslock is on when key entered. Combined with 
                    //the shift button this can result in CapsCheck returning
                    //true even if key entered is lower case.
                    //
                    //The 'CapsNotify' method returns a result based on
                    //the inserted parameter in the form of 
                    //the return of the CapsCheck method
                Console.WriteLine();
                CapsNotify(CapsCheck());                
            }
            while (input != 'Q');
            Console.WriteLine("Program shutting down. Press a key.");
            Console.ReadKey();
        }

        static bool CapsCheck()
        {
                // Check if Capslock is active
                // Return true if Caps is active
            return Console.CapsLock;
        }

        static void CapsNotify(bool caps)
        {
                //Notify whether Capslock is Active
                //In-line check of caps results in accurate output.
            Console.WriteLine("CapsLock is {0}.", (caps ? "Active" : "Inactive"));
        }
    }
}

Again, thank you and feel free to provide more feedback for improvement if such pleases your kind self. ^_^

\$\endgroup\$

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.