Krdytkyu

Follow-up Code: #2 BarTender project with C# - exporting printer code templates

as stated in the question I've got a working C# project.

The project's goal is to open up the application BarTender and export printer code templates (a way of getting information from databases connected to BarTender). I would like to make improvements (i.e. syntax or build quality/speed).

Any general tips or practices would be awesome!

using System;



namespace BarTender

{

    class btExport

    {

        // Dummy Variable

        private const string DataExportPath = "";   



        static void Main(string args)

        {

            ///////////////////////////////////////////////

            /*          VARIABLE DECLARATION            */

            //////////////////////////////////////////////



            // Declare a BarTender application variable

            BarTender.Application btApp;



            // Declare a BarTender document variable

            BarTender.Format btFormat;



            // Declare a BarTender printer code template variable

            BarTender.PrinterCodeTemplate btPCT;



            // Declare a BarTender verification variable

            BarTender.Verification btVerification;



            // Declare a BarTender messages variable to hold all messages

            BarTender.Messages btMessages;



            // Declare a variable to hold message text

            string btMessageText = "";



            // Declare a success variable

            bool ExportSuccess = true;



            // Declare an object variable

            System.Object obj = null;



            // Used for manual input

            string input = null;

            string output = null;







            ///////////////////////////////////////////

            /*          START OF BARTENDER SIDE      */

            ///////////////////////////////////////////



            // Only start BarTender when needed, otherwise use old instances

            try

            {

                // Store this instance as an object variable in C# 



                object btObject = System.Runtime.InteropServices.Marshal.GetActiveObject("BarTender.Application");



                // Convert the object variable to a BarTender application variable 



                btApp = btObject as BarTender.Application;

            }

            catch

            {

                btApp = new BarTender.Application();

            }



            // Set the BarTender application visible

            btApp.Visible = true;













            ////////////////////////////////////////



            /*             START LOGIC        */



            ////////////////////////////////////////



            // If run without parameters this 'if' is triggered for manual entry

            if (args.Length == 0)

            {

                Console.WriteLine("No parameters specified. n Enter manually or try again.");

                Console.Write("Input: ");

                input = Console.ReadLine();

                int start = input.Length - 4;

                output = input.Remove(start, 4);

                output = output += "prn"";

                Console.WriteLine(output);

            }

            // Taking parameters from Main

            else

            {

                input = args[0];

                Console.WriteLine("Input File Path:" + input);

                output = args[1];

                Console.WriteLine("Output File Path:" + output);



            }



            // Open a BarTender document

            try

            {

                btFormat = btApp.Formats.Open(""" + input + """, false, "");



                // Specify the password to remove print-only protection from the application 



                btApp.SpecifyPrintOnlyPassword("#Betsy");



                // Set the printer code template variable

                btPCT = btFormat.PrinterCodeTemplate;



                // Export the printer code template

                btPCT.Export("SAPscript-ITF", BarTender.BtPctExportType.btPctExportCombined, output, DataExportPath, ref obj);

            }

            catch { Console.WriteLine("Input or Export file does not exist."); Console.ReadLine(); return; }



            // Set the messages variable to the object

            btMessages = obj as BarTender.Messages;



            // Check to see if there is an error message

            if (ExportSuccess == false)

            {

                // Loop through the messages

                for (int i = 1; i <= btMessages.Count; i++)

                {

                    // Get the error message

                    btVerification = btMessages.GetMessage(1).Verification;



                    // Populate the error messages into a string

                    btMessageText = btMessageText + btVerification.Problem + "rn" + btVerification.Fields + " " + btVerification.AutoFix + "rn" + btVerification.Result;



                }

            }

            else

            {

                // Loop through the messages

                foreach (BarTender.Message btMsg in btMessages)

                {

                    // Get the error message

                    btVerification = btMessages.GetMessage(1).Verification;



                    // Populate warning messages into a string

                    btMessageText = btMessageText + btVerification.Problem + "rn" + btVerification.Fields + " " + btVerification.AutoFix + "rn" + btVerification.Result;



                }

            }

            // End the BarTender process

            btApp.Quit(BarTender.BtSaveOptions.btDoNotSaveChanges);



            Console.WriteLine("Complete");

 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

        }

    }

}

1. dont prefix your code, bt looks like button... you can just name Format format and be all set. (Clean Code - Uncle Bob)...


2. Break down your ideas into smaller functions (Single Responsibility Principle). like btFormat = btApp.Formats.Open(""" + input + """, false, ""); can be in its own method with a meaningful name. Your btMessageText, can be set from private function called CreateVerificationMessage...


3. what exactly are you going to do with btMessageText? Were you supposed to write that to the console?

Look for "intent" and separate those ideas into functions, methods and classes instead of creating a God Class to do everything...

Bugs and problems:

• 
  input.Remove(start, 4) will throw an exception if start is less than 0, which happens when input.Length is less than 4. You should check for that, or otherwise catch exceptions at that point. This output path logic also seems to be written with very specific inputs in mind - it looks fairly brittle.


• Similarly, args[1] will fail if args.Length is less than 2, but you've only made sure that args.Length isn't 0, so this will throw if only one argument is provided. This too will cause a crash because you're not catching exceptions here.


• Always check for null whenever you use as. You're not doing that with btObject and btMessages, so using them might result in NullReferenceExceptions.


• The final for and foreach loop both seem to do the exact same thing, just in a slightly different way, so one of them can probably be removed. Since ExportSuccess is always true, the for loop is never executed anyway. Either way, both loops always get the same message (index or id 1), which is probably not correct.

Readability:

• There are a lot of unnecessary comments. Things like 'delare a TypeName variable' aren't useful, and there are better ways to get an overview of what code is doing than using 'shouty' headers. Try using comments to explain why code does what it does - what it's doing should be obvious by looking at the code itself.


• As Andy already mentioned, try splitting your Main method up into several methods, each with a specific purpose. GetApplication, GetInputOutputPaths, ExportCodeTemplate and ShowErrorMessages seems like a reasonable way to do it. This keeps Main short and simple, and quickly gives you a high-level overview of what the application is actually doing - without needing any comments.


• Why are all variables declared up-front? I'd move them as close to where each variable is actually used. Preferably in as small a scope as possible, and immediately initialized whenever possible. That keeps related things together, which should make the code easier to understand.


• There's a catch statement body with several statements, including a return, all on a single line. That sort of inconsistencies makes code more difficult to read. Put each of those statements on a separate line.

Other notes:

• 
  btMessageText is not used anywhere. In any case, successive concatenation of strings is more efficiently done by appending to a StringBuilder.


• Things like variable1 + "small string" + variable2 can be written more succinctly with interpolated strings: $"{variable1}small string{variable2}".


• 
  variable == false, where variable is a boolean instead of a nullable boolean, is normally written as !variable.


• Try using more descriptive names. input and output are ok, but inputFilePath and outputFilePath are better. obj and btObject are poor names - messages (or even exportErrorMessages) and application are better. In most cases there is no need to abbreviate things (printerCodeTemplate instead of PCT), and the bt prefix doesn't add any real value and is easily confused with button, as Andy already pointed out.


• 
  btPCT isn't necessary: you can call btFormat.PrinterCodeTemplate.Export(...) directly.


• I'd use using System.Runtime.InteropServices instead of writing Marshal's name out full.


• Why is that password hardcoded instead of passed in as an argument or via user input?


• Why is only one part of the code inside a try-catch statement?


• Personally I would make a lot more use of C#'s type inference (var), especially because in most assignments the type is obvious from both the left and right-hand side.