1
\$\begingroup\$

Follow-up Code: BarTender project - exporting printer code templates - Follow-up

Second Revision: Bartender Project - exporting printer code templates - Follow-up-2

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 + "\r\n" + btVerification.Fields + " " + btVerification.AutoFix + "\r\n" + 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 + "\r\n" + btVerification.Fields + " " + btVerification.AutoFix + "\r\n" + btVerification.Result;

                }
            }
            // End the BarTender process
            btApp.Quit(BarTender.BtSaveOptions.btDoNotSaveChanges);

            Console.WriteLine("Complete");
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! I rolled back your last edit. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ Dec 11 '18 at 20:06
  • \$\begingroup\$ Having section headers like that indicates you should be making functions where the name of those functions are the section headers. I used to do the same thing, I thought it aesthetically looked better, but in reality it made my code harder to read for others (and myself later-on). It makes it harder to get "the big picture" of what the main function is doing when you have to scroll that much. \$\endgroup\$
    – Shelby115
    Dec 14 '18 at 20:43
1
\$\begingroup\$
  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...

\$\endgroup\$
1
  • \$\begingroup\$ I've taken what you said to consideration, I am going to edit the code up top please let me know if you think it looks better! Also: btMessageText was going to display in the console a list of errors that Bartender was generating but seeing as I can't fully figure out how to automate it I can see the errors while running through the program as opposed to reading them in the console. \$\endgroup\$
    – C. Catt
    Dec 11 '18 at 19:55
4
\$\begingroup\$

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.
\$\endgroup\$
1
  • \$\begingroup\$ Couldn't agree more on readability #3. I'm tired of going through legacy code and having to find out 20 minutes later than the only place this variable is used in on the last line. Just declare it when you use it unless you have a reason not to do so. \$\endgroup\$
    – Shelby115
    Dec 14 '18 at 20:46

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.