3
\$\begingroup\$

I'm developing a console app which is supposed to teach how the bitwise operators in C# work. It will include tutorials, training fields and all that good stuff. I have a problem with the naming of my variables. I have a few classes, however, their names are quite long and the variables that I'm passing to the constructor have long names (25-30+ symbols). The main problem is that I'm not even using those class variables. I'm just instantiating them so I can trigger they're constructor which adds them to specific lists and sets a few variables.

How can I shorten the names of those variables? Or even better, how can I remove them completely and still use some method similar to they're constructors?

This is the main class here I create the variables and do my core logic:

public class MainScreen
{
    private static readonly string[] welcomeMessage =
    {
        "Welcome to the Bitwise Operator Trainer !",
        "This program's purpose is to teach you how the C# bitwise operator work.",
        "It contains a training field and tutorials for each operator. ",
        "To check all the available commands type /help"
    };
    public static readonly List<ICommand> TutorialCommandsList = new List<ICommand>();
    public static readonly List<ICommand> AllCommandsList = new List<ICommand>();
    public static readonly List<ICommand> TrainingCommandsList = new List<ICommand>();
    private static readonly List<char[]> distincitiveInputs = new List<char[]>();

    private static readonly string[] welcomeMessage =
    {
        "Welcome to the Bitwise Operator Trainer !",
        "This program's purpose is to teach you how the C# bitwise operator work.",
        "It contains a training field and tutorials for each operator. ",
        "To check all the available commands type /help"
    };

    private static readonly string[] _helpCommandAccessors = { "/help", "/h", "/?" };
    private const string _helpCommandInfo = "Comamnd to show the help menu";
    private static readonly HelpCommand helpCommand = new HelpCommand(_helpCommandAccessors, _helpCommandInfo);


    private static readonly string[] _trainingBinaryANDAccessors = { "/train &", "/train Binary AND" };
    private const string _trainingBinaryANDInfo = @"Training field for the Binary AND ""&"" operator";
    private static readonly TrainingBinaryAND TraingingTrainingBinaryAND = new TrainingBinaryAND(_trainingBinaryANDAccessors, _trainingBinaryANDInfo);

    private static readonly string[] _trainingBinaryORAccessors = {"/train |", "/train Binary OR"};
    private const string _trainingBinaryORInfo = @"Training field for the Binary OR ""|"" operator";
    private static readonly TrainingBinaryOR TrainingBinaryOR = new TrainingBinaryOR(_trainingBinaryORAccessors, _trainingBinaryORInfo);

    private static readonly string[] _trainingBinaryXORAccessors = {"/train ^", "/train Binary XOR"};
    private const string _trainingBinaryXORInfo = @"Training field for the Binary XOR ""^"" operator";
    private static readonly TrainingBinaryXOR TrainingBinaryXOR = new TrainingBinaryXOR(_trainingBinaryXORAccessors,_trainingBinaryXORInfo);

    private static readonly string[] _trainingBinaryLeftShiftAccessors = { "/train <<", "/train Binary LeftShift" };
    private const string _trainingBinaryLeftShiftInfo = @"Training field for the Binary Left Shift ""<<"" operator";
    private static readonly TrainingBinaryLeftShift TrainingBinaryLeftShift = new TrainingBinaryLeftShift(_trainingBinaryLeftShiftAccessors, _trainingBinaryLeftShiftInfo);

    private static readonly string[] _trainingBinaryRightShiftAccessors = { "/train >>", "/train Binary RightShift" };
    private const string _trainingBinaryRightShiftInfo = @"Training field for the Binary Right Shift "">>"" operator";
    private static readonly TrainingBinaryRightShift TrainingBinaryRightShift = new TrainingBinaryRightShift(_trainingBinaryRightShiftAccessors, _trainingBinaryRightShiftInfo);

    private static readonly string[] _trainingBinaryCircualrLeftShiftAccessors = {"/train <<|>>", "/train Binary Left Circular Shift" };
    private const string _trainingBinaryCircualrLeftShiftInfo = @"Training field for the Binary Left Circular Shift ""<< | >>"" operator";
    private static TrainingBinaryCircularLeftShift TrainingBinaryCircularShift = new TrainingBinaryCircularLeftShift(_trainingBinaryCircualrLeftShiftAccessors, _trainingBinaryCircualrLeftShiftInfo);

    private static readonly string[] _trainingBinaryCircularRightShiftAccessors = {"/train >>|<<","/train Binary Right Circular Shift"};
    private const string _trainingBinaryCircularRightShiftInfo = @"Training field for the Binary Right Circular Shift "">> | <<"" operator";
    private static TrainingBinaryCircularRightShift TrainingBinaryCircularRightShift = new TrainingBinaryCircularRightShift(_trainingBinaryCircularRightShiftAccessors, _trainingBinaryCircularRightShiftInfo);


    private static readonly string[] _generalBinaryANDAccessors = {"/Binary AND", "/&"};
    private const string _generalBinaryANDInfo = @"General Information about the Binary AND operator ""&"" ";
    private static readonly GeneralBinaryAND generalBinaryANDCommand = new GeneralBinaryAND(_generalBinaryANDAccessors,_generalBinaryANDInfo);

    private static readonly string[] _generalBinaryORAccessors = {"/Binary OR", "/|"};
    private const string _generalBinaryORInfo = @"General Information about the Binary OR operator ""|"" ";
    private static readonly GeneralBinaryOR generalBinaryORCommand = new GeneralBinaryOR(_generalBinaryORAccessors, _generalBinaryORInfo);

    private static readonly string[] _generalBinaryXORAccessors = { "/Binary XOR", "/^" };
    private const string _generalBinaryXORInfo = @"General Information about the Binary XOR operator ""^"" ";
    private static readonly GeneralBinaryXOR generalBinaryXORCommand = new GeneralBinaryXOR(_generalBinaryXORAccessors, _generalBinaryXORInfo);

    private static readonly string[] _generalBinaryLeftShiftAccessors = {"/Binary Left Shift", "/<<"};
    private const string _generalBinaryLeftShiftInfo = @"General Information about the Binary Left Shift operator ""<<"" ";
    private static readonly GeneralBinaryLeftShift generalBinaryLeftShift = new GeneralBinaryLeftShift(_generalBinaryLeftShiftAccessors, _generalBinaryLeftShiftInfo);

    private static readonly string[] _generalBinaryRightShiftAccessors = {"/Binary Right Shift", "/>>"};
    private const string _generalBinaryRightShiftInfo = @"General Information about the Binary Right Shift operator "">>"" ";
    private static readonly GeneralBinaryRightShift GeneralBinaryRightShift = new GeneralBinaryRightShift(_generalBinaryRightShiftAccessors,_generalBinaryRightShiftInfo);

    private static readonly string[] _generalBinaryCircularShiftAccessors = {"/Binary Circular Shift", "/<<|>>"};
    private const string _generalBinaryCircularShiftInfo = @"General Information about the Binary Circular Shift operator ""<< | >>"" ";
    private static readonly GeneralBinaryCircularShift GeneralBinaryCircularShift = new GeneralBinaryCircularShift(_generalBinaryCircularShiftAccessors,_generalBinaryCircularShiftInfo);

    private static readonly string[] _generalBinaryNOTAccessors = {"/Binary NOT", "/~"};
    private const string _generalBinaryNOTInfo = @"General Information about the Binary NOT operator ""~"" ";
    private static readonly GeneralBinaryNOT GeneralBinaryNot = new GeneralBinaryNOT(_generalBinaryNOTAccessors,_generalBinaryNOTInfo);

    private static readonly string[] _generalBitwiseAccessors = {"/Bitwise", "/BW"};
    private const string _generalBitwiseInfo = @"General Information about Bitwise Operators";
    private static readonly GeneralBitwiseOperators GeneralBitwiseOperators = new GeneralBitwiseOperators(_generalBitwiseAccessors,_generalBitwiseInfo);

    private static void Main()
    {
        Console.ForegroundColor = Settings.DefaultColor;
        AddDistincitiveInput(Settings.TutorialsDistinctive, distincitiveInputs);
        AddDistincitiveInput(Settings.TrainingDistinctive, distincitiveInputs);

        foreach (string text in welcomeMessage)
        {
            Console.WriteLine(text);
        }
        Console.WriteLine();

        while (true)
        {
            string input = Console.ReadLine();
            bool isHalfCommand = HalfCommand(input).Item1;
            int numberOfCommand = HalfCommand(input).Item2;
            if (isHalfCommand)
            {
                if (numberOfCommand == (int) Settings.Distinctives.Tutorials)
                {
                    WriteCommands(TutorialCommandsList);
                }
                else if (numberOfCommand == (int) Settings.Distinctives.Training)
                {
                    WriteCommands(TrainingCommandsList);
                }
            }
            else if (IsCommand(input))
            {
                string command = input; // smenqm imeto poneje veche znaem che e vuvedena commanda;
                foreach (
                    var currentCommand in
                        AllCommandsList.Where(currentCommand => currentCommand.IsThisCommand(command)))
                {
                    currentCommand.Display();
                    Console.WriteLine();
                }
            }
            else
            {
                Console.WriteLine("Invalid command");
            }
        }
    }

    public static bool IsCommand(string inputText)
    {
        char[] charInput = inputText.ToCharArray();
        if (charInput.Length <= 0) return false;
        if (charInput[0] != '/') return false;
        string command = charInput.Aggregate("", (current, t) => current + t);
        return IsContained(command);
    }
    private static bool IsContained(string inputCommand)
    {
        return
            AllCommandsList.Select(t => t.CommandAccessor.Contains(inputCommand))
                .Where((b1, i) => i >= AllCommandsList.Count - 1 || b1)
                .FirstOrDefault();
    }

    private static Tuple<bool,int> HalfCommand(string inputText)
    {
        char[] charInput = inputText.ToCharArray();
        Tuple<bool, int> retTuple = new Tuple<bool, int>(false, 0);
        for (int i = 0; i < distincitiveInputs.Count; i++)
        {
            var same = 0;
            char[] distinctiveInput = distincitiveInputs[i];
            if (charInput.Length == distinctiveInput.Length)
            {
                same += distinctiveInput.Where((t, j) => charInput[j] == t).Count();
            }
            if (same == distinctiveInput.Length)
            {
                return new Tuple<bool, int>(true, i);
            }

        }
        return retTuple;
    }

    public void AddCommand(ICommand newCommand)
    {
        AllCommandsList.Add(newCommand);
    }
    public void AddCommand(ICollection<ICommand> commandList, ICommand newCommand)
    {
        AllCommandsList.Add(newCommand);
        commandList.Add(newCommand);
    }

    private static void AddDistincitiveInput(string inputText, ICollection<char[]> distinctiveCollection)
    {
        distinctiveCollection.Add(inputText.ToCharArray());
    }

    public static void WriteCommands(IEnumerable<ICommand> listOfCommands)
    {
        Console.WriteLine();
        foreach (var command in listOfCommands)
        {
            Console.ForegroundColor = ConsoleColor.Yellow;
            Console.WriteLine(command.CommandInfo);
            Console.ForegroundColor = ConsoleColor.DarkYellow;
            foreach (string commandAccesor in command.CommandAccessor)
            {
                Console.WriteLine("{0}", commandAccesor);
            }
            Console.WriteLine();
        }
        Console.WriteLine();
        Console.ForegroundColor = Settings.DefaultColor;
    }
}

CommandAccessors are used to detect if the user has entered some of the listed commands for example if the user wants to see the help menu one types one of the 3 available commands.

CommandInfo is being displayed when the help menu is shown so that the user know what the commands do.

The class itself is not being used as I mentioned before. It's just there so I can trigger its constructor.

I have one collection which holds all the available commands List<ICommand> AllCommandsList = new List<ICommand>(); and outputs them when called by /help.

There are also two more lists which are there to separate the commands. One of them holds the tutorial commands and the other one - the training ones List<ICommand> TutorialCommandsList = new List<ICommand>(); and List<ICommand> TrainingCommandsList = new List<ICommand>();.

Now about the classes. Every class implements an interface called ICommand:

public interface ICommand
{
    string[] CommandAccessor { get; set; }
    string CommandInfo { get; set; }

    bool IsThisCommand(string inputCommand);

    void Display();
}

This is a simple interface that contains only the most essential data for one of my classes. For now I have created only the General and the Training classes so i still don't have implementation of any of the tutorials so i will just skip them and try to cover the trainingClasses. Every training class needs to inherit two interfaces: the ICommand and one more interface which is specific for the training class.

public interface ITraining
{
    void DisplayWelcomeMessage(string operation, string operationLetters);

    void CheckAnswer(int result, int a, int b);

    void DisplayTraining(string operation, string operationLetters);

    bool GetValue(string tempInput, ref int b);

    int BitOperationResult(int input1, int input2);

    Func<int, int, int> BitOperator { get;}
}

Here we have Func<int,int,int>. This is used to pass a bitwise operator later on. Now to the last part. We have an abstract class which inherits the two interfaces and implements some of the functions. It also leaves some of the voids open to implement as abstract voids.

public abstract class Training :ITraining, ICommand
{
    public abstract Func<int, int, int> BitOperator { get ; }

    public string[] CommandAccessor { get; set; }
    public string CommandInfo { get; set; }

    private readonly MainScreen ms = new MainScreen();

    protected Training(string[] commandAccesors, string commandInfo)
    {
        CommandAccessor = commandAccesors;
        CommandInfo = commandInfo;
        ms.AddCommand(MainScreen.TrainingCommandsList, this);
    }
    public bool GetValue(string tempInput, ref int b)
    {
        if (tempInput == "exit" || MainScreen.IsCommand(tempInput))
        {
            Console.WriteLine("You exited succesfully");
            return true;
        }
        while (!int.TryParse(tempInput, out b))
        {
            Console.ForegroundColor = ConsoleColor.Red;
            Console.WriteLine("Input can be only numbers !");
            Console.ForegroundColor = Settings.DefaultColor;
            tempInput = Console.ReadLine();
        }
        return false;
    }

    public int BitOperationResult(int input1, int input2)
    {
        return BitOperator(input1, input2);
    }

    public void DisplayTraining(string operation, string operationLetters)
    {
        DisplayWelcomeMessage(operation, operationLetters);
        while (true)
        {
            int a = 0;
            int b = 0;
            var result = 0;
            Console.Write("Enter a = ");
            string tempInput = Console.ReadLine();
            if (GetValue(tempInput, ref a)) break;
            Console.Write("Enter b = ");
            tempInput = Console.ReadLine();
            if (GetValue(tempInput, ref b)) break;
            Console.WriteLine("The equation looks like this now : {0} {2} {1}", a, b,operation);
            Console.Write("Now enter the answer you think is right : {0} {2} {1} = ", a, b,operation);
            tempInput = Console.ReadLine();
            if (GetValue(tempInput, ref result)) break;

            CheckAnswer(result, a, b);
            Console.ForegroundColor = Settings.DefaultColor;
            Console.WriteLine("Now let's try again");
            Console.WriteLine();
        }
    }

    public void CheckAnswer(int result, int a, int b)
    {
        if (result == BitOperator(a, b))
        {
            Console.ForegroundColor = ConsoleColor.Green;
            Console.Write("Correct ! ");
            Console.Write("The answer is = {0} ", result);
        }
        else
        {
            Console.ForegroundColor = ConsoleColor.Red;
            Console.WriteLine("Wrong .. the correct answer is {0} ", BitOperator(a, b));
        }
    }

    public void DisplayWelcomeMessage(string operation, string operationLetters)
    {
        string[] welcomeMessage =
{
            @"Welcome to the Testing Area for the " +operationLetters + " " + operation + " .",
            @"Here you can enter 2 values and try to calculate the result.",
            @"A message will be shown whether you got the correct answer or no."
        };
        foreach (var s in welcomeMessage)
        {
            Console.WriteLine(s);
        }
        Console.WriteLine(
        @"You can exit at any time by typing exit or redirect yourself to any other tutorial/training");
        Console.WriteLine();
        Console.WriteLine("This is how the {0} operation looks like : a {1} b", operationLetters, operation);
    }

    public bool IsThisCommand(string inputCommand)
    {
        return CommandAccessor.Contains(inputCommand);
    }

    public abstract void Display();  
}

Now every single Training class just inherits the final abstract class just like this:

    public class TrainingBinaryAND : Training
{
    public TrainingBinaryAND(string[] commandAccesors, string commandInfo) : base(commandAccesors, commandInfo)
    {
    }

    public override Func<int, int, int> BitOperator { get; } = (a, b) => a & b;

    public override void Display()
    {
        DisplayTraining("&", "Binary AND");
    }
}

Nothing that much to say here just overriding the Display void and giving the proper bitwise operator to the Func<int,int,int>.

Now only the general classes are left:

  • They all inherit an abstract class called General which on the other hand inherits ICommand interface.

  • They implement just one method Display:

    public abstract class General : ICommand
    {
        private readonly MainScreen ms = new MainScreen();
        protected General(string[] commandAccesors, string commandInfo)
        {
            CommandAccessor = commandAccesors;
            CommandInfo = commandInfo;
            ms.AddCommand(this);
        }
    
        public string[] CommandAccessor { get; set; }
        public string CommandInfo { get; set; }
        public bool IsThisCommand(string inputCommand)
        {
            return CommandAccessor.Contains(inputCommand);
        }
    
        public abstract void Display();
    }
    

The general and the training classes have quite similar constructors. In the first place I'm instantiating the class variables with the long names just so I can use the constructor. And similar to the training class, which also leaves the Display method open to implement by the derived classes:

public class GeneralBinaryLeftShift : General
{
    public GeneralBinaryLeftShift(string[] commandAccesors, string commandInfo) : base(commandAccesors, commandInfo)
    {
    }

    public override void Display()
    {
        Console.WriteLine(@"Binary Left Shift Operator ""<<"". The left operands value is moved left by the number of bits specified by the right operand.");
    }
}

That's pretty much of it. I have a few more classes which are similar to those shown above, so I won't post them here. But I will post some of the additional classes like Settings which I use in basically every class at least once.

public static class Settings
{
    public enum Distinctives
    {
        Tutorials,
        Training
    }
    public const ConsoleColor DefaultColor = ConsoleColor.White;
    public const string TutorialsDistinctive = "/tutorial";
    public const string TrainingDistinctive = "/training";
}
\$\endgroup\$
3
\$\begingroup\$

To start with, your Main class does too many things at once. Amongst others:

  • Main holds the user-visible texts
  • Main defines which trainings exit
  • Main defines the logic to run trainings

What about breaking these distinct tasks into different classes?

Texts

You could keep all user-visible strings in one class dedicated to just this task (Single Responsibility Principle). Keeping this class non-static will allow you to add translations to your app in the future -- by simply adding a second, translated instance of Texts (Open-Closed-Principle).

This cleans up Main and reliefs it of some of its duties. As an addition, you could later easily introduce an interface ITexts, or have the implementation of Texts use a external resource file.

The Texts-class could look something like this:

class Texts {
    public string WelcomeMessage => "Welcome [...]";
    public string BinaryANDInfo => @"General Information about the Binary AND operator ""&"" ";
    // ... and so on ...
}   

Defining the Trainings

The thing that strikes me here is that you define a separate subclass of Training for each and every training (TrainingBinaryAND, TrainingBinaryCircularRightShift, and many many more). This seems a bit unpractical to me, especially since the class you listed (TrainingBinaryAND) does nothing that would require a separate class definition.

A guideline that comes to mind is "Prefer composition to inheritance". Using that approach, we could introduce a TrainingDefinition class, and have each training be just an instance of this. Training's constructor would be changed from

protected Training(string[] commandAccesors, string commandInfo)

to:

protected Training(TrainingDefinition definition)

with the TrainingDefinitionlooking something like this:

public class TrainingDefinition 
{
    public string[] CommandAccessor { get; set; }
    public string CommandInfo { get; set; }
    public string Operation { get; set; }
    public string OperationLetters { get; set; }
    public Func<int, int, int> BitOperator { get; set; }
} 

Then, in your so-far abstract Training class, you can just reference the definitions in the places that used to be abstract:

public class Training : ITraining, ICommand
{
    public Func<int, int, int> BitOperator => definition.BitOperator;
    public string[] CommandAccessor
    {
        get { return definition.CommandAccessor; }
        set { definition.CommandAccessor = value; }
    }

    public string CommandInfo
    {
        get { return definition.CommandInfo; }
        set { definition.CommandInfo = value; }
    }

    private TrainingDefinition definition;

    public Training(TrainingDefinition definition)
    {
        this.definition = definition;
    }

    // rest of the class unchanged ...

    public void Display()
    {
        DisplayTraining(definition.Operation, definition.OperationLetters);
    }
}

This would allow you to get rid of all the boilerplate classes like TrainingBinaryAND and TrainingBinaryCircularRightShift. Your general Trainingclass can now handle them all.

To clean up your Main class, you could then add a new class dedicated to the definitions of all existing trainings. Something like this:

public class TrainingDefinitions 
{
    public IEnumerable<TrainingDefinition> AllTrainings => new[] {
            BinaryAND, BinaryOR, // Add more here
    };

    public TrainingDefinition BinaryAND => new TrainingDefinition 
        {
            CommandAccesors = new [] {"/train &", "/train Binary AND"}
            CommandInfo = @"Training field for the Binary AND ""&"" operator"; 
            Operation = "&",
            OperationLetters = "Binary AND",
            BitOperator = (a, b) => a & b,
        };

    public TrainingDefinition BinaryOR => new TrainingDefinition
        {
            /* ... as above ... */
        };

    // and so on ...
}

This will allow you to create all the trainings in a normal foreach-lopp, maybe like this:

void Main() 
{
    // ...
    var mainScreen = new MainScreen();
    var definitions = new TrainingDefinitions().AllTrainings;

    foreach (var definition in definitions) 
    {
        var training = new Training(definiton);
        mainScreen.AddCommand(MainScreen.TrainingCommandsList, training);
    }
    // ...
}

If you do that, you'll get rid of most the static variables in Main.

You'll probably be able to refactor the General classes (the ones that print general information) in much the same way.

Doing this will remove all of these long-named static variable names from main :)

Hope this helps! Have fun :)

|improve this answer|||||
\$\endgroup\$
  • 2
    \$\begingroup\$ I would move the text to a resource (.resx) file. It gives you the same separation of concerns and provides support for multiple languages, but it's more standard. \$\endgroup\$ – Dan Lyons Mar 21 '16 at 18:02
  • \$\begingroup\$ Hello, thank you for the response, however i'm still beginner and there are few things in your code that i don't understand.First the training constructor should it still contain the setting of the CommandAccessors variable ? I changed it to this CommandAccessor = definition.CommandAccesors;. Second i really dont understand what you mean by this : " Then, in your so-far abstract Training class, you can just reference the definitions in the places that used to be abstract." Lastly how does TrainingDefinitions class help me ? \$\endgroup\$ – Denis Mar 21 '16 at 18:58
  • \$\begingroup\$ @DanLyons: That makes perfect sense. \$\endgroup\$ – Felix Alcala Mar 21 '16 at 19:11
  • \$\begingroup\$ @denis: I've expanded the answer to elaborate on these questions. \$\endgroup\$ – Felix Alcala Mar 21 '16 at 19:31
  • \$\begingroup\$ @FelixAlcala one last question what's the point of the Text class if we also put the CommandInfo string into the TrainingDefinitions class :? \$\endgroup\$ – Denis Mar 21 '16 at 19:55

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.