3
\$\begingroup\$

Background

Some very good observations were made about my original code in the 2 answers to this question. In this version I have attempted to reduce cyclic complexity and class coupling even more as well as improve maintainability. The worst offenders of maintainability are now Visual Studio generated modules for the dialogs. Hopefully this is a much more SOLID implementation, please let me know where I violate any of the SOLID principles.

If you wrote one of the 2 answers to the previous question and do not see where I addressed your concerns please let me know, I may have overlooked it, or I will explain why I didn't implement it. I did base the ExcelInterface class on the IDisposable interface, it still didn't clear up all the orphan processes, but I have addressed that in the protected virtual void Dispose(bool disposing) function.

One bad habit I do acknowledge is there is currently no unit testing, only testing of the application itself.

The code behind the forms for this question are in The User Interface Code since I can't fit all the code into a single question.

The entire source for the project can be found in my GitHub Repository. It also contains a CSV version of the test data.

All issues from this review and the user interface review can be found recorded in the issues list on the GitHub repository.

The differences from the original implementation:

  • The global static variable declared in Program have been migrated to a public static class called Globals.
  • Most MessageBox instances have been moved into the UI portion of the program.
  • Each access of the data in the Excel Workbook is a stand alone task and the workbook is closed and the excel process is killed to prevent orphan excel processes from being left behind.
  • Two Exception classes have been added to improve error handling.
  • The class CExcelInteropMethods that interfaces with excel has been broken into multiple classes and is no longer monolithic, unfortunately it is still 390 lines of code, the new classes are ExcelInterface, ExcelFileData, CheckExcelWorkBookOpen, ExcelFileException, AlreadyOpenInExcelException and TenantDataTable. Some portions of the original CExcelInteropMethods have been moved into the Globals class or added as variables to the Globals class.
  • The class CRenter has been replaced by Tenant.
  • The bad C++ programming habit of classes starting with C has been removed, however, the code is now in 2 subdirectories, one called Classes and one called Forms. It helps me to find the code I need to work on.

Questions/Concerns

I am still interested in:

  • Maintainability
  • Cyclic Complexity
  • Class/Object Coupling
  • Anything I am doing in C# that is considered a bad habit.
  • Anything where I can decrease the amount of code by using LINQ.

First 20 lines of Analysis

Here is the first 20 lines of the code analysis as a CSV file. The CVS file is posted on GitHub if you want to see all of it. It has been sorted first by Maintainability, then by Complexity and then by Class Coupling.

Scope,Project,Namespace,Type,Member,Maintainability Index,Cyclomatic Complexity,Depth of Inheritance,Class Coupling,Lines of Source code,Lines of Executable code
Assembly,TenantRosterAutomation (Release), , , ,76,435,7,130,3806,1548
Namespace,TenantRosterAutomation (Release),TenantRosterAutomation, , ,76,429,7,119,3707,1531
Member,TenantRosterAutomation (Release),TenantRosterAutomation,EditPreferencesDlg,InitializeComponent() : void,25,1,,13,217,156
Member,TenantRosterAutomation (Release),TenantRosterAutomation,AddOrEditResidentDlg,InitializeComponent() : void,25,1,,10,238,162
Member,TenantRosterAutomation (Release),TenantRosterAutomation,PrintMailboxListsDlg,InitializeComponent() : void,32,1,,12,128,91
Member,TenantRosterAutomation (Release),TenantRosterAutomation,DeleteRenterDlg,InitializeComponent() : void,34,1,,12,118,81
Member,TenantRosterAutomation (Release),TenantRosterAutomation,RentRosterApp,InitializeComponent() : void,37,1,,9,94,63
Member,TenantRosterAutomation (Release),TenantRosterAutomation,ApartmentNumberVerifier,InitializeComponent() : void,45,1,,10,55,33
Type,TenantRosterAutomation (Release),TenantRosterAutomation,AddOrEditResidentDlg, ,51,15,7,20,349,199
Member,TenantRosterAutomation (Release),TenantRosterAutomation,TenantDataTable,"UdateTenantDataTable(int, Tenant) : bool",52,1,,4,31,20
Member,TenantRosterAutomation (Release),TenantRosterAutomation,PrintMailboxListsDlg,printAndOrSaveMailList(int) : void,53,10,,7,30,17
Member,TenantRosterAutomation (Release),TenantRosterAutomation,ExcelInterface,SaveEdits(List<Apartment>) : void,53,3,,10,39,20
Member,TenantRosterAutomation (Release),TenantRosterAutomation,MSWordInterface,"CreateMailistPrintAndOrSave(string, MailboxData, bool, bool, bool, bool) : bool",53,2,,8,38,19
Member,TenantRosterAutomation (Release),TenantRosterAutomation,MSWordInterface,"AddTitleToMailBoxList(Document, int, bool) : void",54,2,,8,23,17
Member,TenantRosterAutomation (Release),TenantRosterAutomation,ReportCurrentStatusWindow,InitializeComponent() : void,55,1,,8,33,17
Member,TenantRosterAutomation (Release),TenantRosterAutomation,UserPreferences,GetPreferenceValues(string[]) : bool,56,10,,5,54,12
Member,TenantRosterAutomation (Release),TenantRosterAutomation,PropertyComplex,CreateBuildingList(List<BuildingAndApartment>) : void,56,5,,4,32,15
Member,TenantRosterAutomation (Release),TenantRosterAutomation,PropertyComplex,"VerifyApartmentNumber(string, int) : PropertyComplex.ApartmentNumberValid",56,5,,3,34,16
Member,TenantRosterAutomation (Release),TenantRosterAutomation,UserPreferences,SavePreferencesToFile(string) : bool,57,4,,3,30,15
Member,TenantRosterAutomation (Release),TenantRosterAutomation,ExcelFileData,GetActiveWorkSheetContents(bool) : DataTable,57,3,,5,36,15
Member,TenantRosterAutomation (Release),TenantRosterAutomation,AddOrEditResidentDlg,"AddNewResident_Form_Load(object, EventArgs) : void",57,2,,5,18,13
Type,TenantRosterAutomation (Release),TenantRosterAutomation,EditPreferencesDlg, ,59,30,7,25,412,213
Member,TenantRosterAutomation (Release),TenantRosterAutomation,MSWordInterface,"FillFloor1Stand2Nd(MailboxData, int, Table) : void",59,4,,5,24,12

The Code

To allow for some comparison with the original code I will do my best to present the modules in the same order as before, modules that haven't changed will not be presented here.

ExcelInterface.cs

using System;
using System.Collections.Generic;
using System.Data;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Windows;
using Excel = Microsoft.Office.Interop.Excel;

namespace TenantRosterAutomation
{
    // Provides the interface between the rest of the program and Microsoft excel.
    // This class is constructed on an as needed basis and should not persist
    // during execution. The distruction of this class releases the excel process
    // of this class creates and prevents orphan excel processes from being
    // created.
    class ExcelInterface : IDisposable 
    {
        private Excel.Application xlApp;
        private Excel.Workbook xlWorkbook;
        private Excel.Worksheet tenantRoster;

        private bool disposed;
        private string tenantRosterName;
        private string WorkbookName;

        public int ExcelProcessId { get; private set; }

        public ExcelInterface(string workBookName, string workSheetName)
        {
            if (string.IsNullOrEmpty(workBookName))
            {
                ExcelFileException efe =
                    new ExcelFileException("Attempted to create ExcelInterface " +
                    "Object without Excel Data File Name.");
                throw efe;
            }
            WorkbookName = workBookName;
            tenantRosterName = workSheetName;
        }

        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        public void SaveEdits(List<Apartment> tenantUpdates)
        {
            ReportCurrentStatusWindow SaveStatus = new ReportCurrentStatusWindow();
            string eSaveMsg = "Can't save edits to " + WorkbookName;
            try
            {
                SaveStatus.MessageText = "Saving updated tenants and apartments to Excel.";
                SaveStatus.Show();
                StartExcelOpenWorkbook();
                OpenTenantRosterWorkSheet();
                xlApp.Visible = false;
                xlApp.DisplayAlerts = false;

                if (tenantRoster == null)
                {
                    MessageBox.Show(eSaveMsg + " can't open the excel worksheet "
                        + tenantRosterName + " to save changes");
                    return;
                }
                List<string> columnNames = GetColumnNames();
                foreach (Apartment edit in tenantUpdates)
                {
                    UpdateColumnData(edit, columnNames);
                }
                xlWorkbook.Save();
                SaveStatus.Close();
            }
            catch (AlreadyOpenInExcelException)
            {
                SaveStatus.Close();
                throw;
            }
            catch (Exception ex)
            {
                SaveStatus.Close();
                ExcelFileException efe = new ExcelFileException(eSaveMsg, ex);
                throw efe;
            }
        }

        public List<string> GetWorkSheetNames()
        {
            List<string> sheetNames = null;

            StartExcelOpenWorkbook();
            if (xlWorkbook != null)
            {
                sheetNames = new List<string>();

                int SheetCount = xlWorkbook.Sheets.Count;
                for (int i = 1; i <= SheetCount; ++i)
                {
                    string sheetname = xlWorkbook.Sheets[i].Name;
                    sheetNames.Add(sheetname);
                }
            }

            return sheetNames;
        }

        public DataTable GetWorkSheetContents()
        {
            StartExcelOpenWorkbook();
            int headerRow = 1;
            int firstColumn = 1;
            DataTable workSheetContents = ReadExcelIntoDatatble(headerRow, firstColumn);

            return workSheetContents;
        }

        protected virtual void Dispose(bool disposing)
        {
            if (!disposed)
            {
                if (disposing)
                {
                    if (xlWorkbook != null)
                    {
                        xlWorkbook.Close();
                        xlWorkbook = null;
                    }

                    if (xlApp != null)
                    {
                        xlApp.Quit();
                        xlApp = null;
                        // Ensure that Excel exits after this class is disposed.
                        // In some cases orphaned Excel proceses remain.
                        Process xlProcess = Process.GetProcessById(ExcelProcessId);
                        if (xlProcess != null)
                        {
                            xlProcess.Kill();
                        }
                    }
                }
                disposed = true;
            }
        }

        [DllImport("user32.dll")]
        static extern int GetWindowThreadProcessId(int hWnd, out int lpdwProcessId);
        private int GetExcelProcessID(Excel.Application excelApp)
        {
            int processId;
            GetWindowThreadProcessId(excelApp.Hwnd, out processId);
            return processId;
        }

        private void StartExcelOpenWorkbook()
        {
            if (xlApp != null)
            {
                return;
            }

            CheckExcelWorkBookOpen testOpen = new CheckExcelWorkBookOpen();
            testOpen.TestAndThrowIfOpen(WorkbookName, false);
            xlApp = new Excel.Application();
            xlApp.Visible = false;
            xlApp.DisplayAlerts = false;

            xlWorkbook = xlApp.Workbooks.Open(WorkbookName);

            // Retain the process ID so that the process can be killed later
            ExcelProcessId = GetExcelProcessID(xlApp);
        }

        // Updates a row of data in the excel file
        private void UpdateColumnData(Apartment rowEdit, List<string> columnNames)
        {
            Tenant tenant = rowEdit.Tenant;
            Excel.Range currentRow = FindRowInWorkSheetForUpdate(rowEdit.ApartmentNumber);
            UpdateColumn(currentRow, "Last", tenant.LastName, columnNames);
            UpdateColumn(currentRow, "First", tenant.FirstName, columnNames);
            UpdateColumn(currentRow, "Add OCC First", tenant.CoTenantFirstName, columnNames);
            UpdateColumn(currentRow, "Add OCC Last", tenant.CoTenantLastName, columnNames);
            UpdateColumn(currentRow, "Ph #", tenant.HomePhone, columnNames);
            UpdateColumn(currentRow, "Renters Ins", tenant.RentersInsurancePolicy, columnNames);
            UpdateColumn(currentRow, "Lease Start", tenant.LeaseStart, columnNames);
            UpdateColumn(currentRow, "Lease End", tenant.LeaseEnd, columnNames);
            UpdateColumn(currentRow, "Email", tenant.Email, columnNames);
        }

        private void OpenTenantRosterWorkSheet()
        {
            try
            {
                if (!string.IsNullOrEmpty(tenantRosterName))
                {
                    List<string> sheetNames = GetWorkSheetNames();
                    bool exists = sheetNames.Any(x => x.Contains(tenantRosterName));
                    if (!exists)
                    {
                        MessageBox.Show("The workbook " + WorkbookName + " does not contain the worksheet " + tenantRosterName);
                        tenantRoster = null;
                    }
                    else
                    {
                        tenantRoster = xlWorkbook.Worksheets[tenantRosterName];
                    }
                }
            }
            catch (Exception ex)
            {
                string eMsg = "Function ExcelInterface.OpenTenantRosterWorkSheet() failed! ";
                ExcelFileException efe = new ExcelFileException(eMsg, ex);
                throw efe;
            }
        }

        // To enhance performance the excel worksheet is read once into a local
        // DataTable.
        private DataTable ReadExcelIntoDatatble(int HeaderLine, int ColumnStart)
        {
            try
            {
                OpenTenantRosterWorkSheet();
                if (tenantRoster == null)
                {
                    return null;
                }

                Excel.Range TenantDataRange = tenantRoster.UsedRange;
                DataTable tenantTable = CreateDataTableAddColumns(TenantDataRange,
                    HeaderLine, ColumnStart);
                AddTenantDataToTenantTable(TenantDataRange, ref tenantTable,
                    HeaderLine, ColumnStart);

                return tenantTable;
            }
            catch (Exception ex)
            {
                string eMsg = "In ExcelInterface::ReadExcelToDatatble error! ";
                ExcelFileException efe = new ExcelFileException(eMsg, ex);
                throw efe;
            }
        }

        private DataTable CreateDataTableAddColumns(Excel.Range TenantDataRange,
            int headerLine, int firstColumn)
        {
            DataTable tenantTable = new DataTable();
            int columnCount = TenantDataRange.Columns.Count;

            for (int column = firstColumn; column <= columnCount; column++)
            {
                tenantTable.Columns.Add(Convert.ToString
                    (TenantDataRange.Cells[headerLine, column].Value2), typeof(string));
            }

            return tenantTable;
        }

        private void AddTenantDataToTenantTable(Excel.Range TenantDataRange,
            ref DataTable tenantTable, int headerLine, int firstColumn)
        {
            int columnCount = TenantDataRange.Columns.Count;
            int rowcount = TenantDataRange.Rows.Count;

            var values = TenantDataRange.Value2;
            for (int row = headerLine + 1; row <= rowcount; row++)
            {
                DataRow tenantData = tenantTable.NewRow();
                for (int column = firstColumn; column <= columnCount; column++)
                {
                    tenantData[column - firstColumn] =
                        Convert.ToString(values[row, column]);
                }
                tenantTable.Rows.Add(tenantData);
            }
        }

        private Excel.Range FindRowInWorkSheetForUpdate(int apartmentNumber)
        {
            Excel.Range currentRow = null;
            object oMissing = Missing.Value;

            try
            {
                Excel.Range UnitNoColumn = GetUnitColumn();
                if (UnitNoColumn != null)
                {
                    currentRow = UnitNoColumn.Find(apartmentNumber.ToString(), oMissing,
                        Excel.XlFindLookIn.xlValues, Excel.XlLookAt.xlPart,
                        Excel.XlSearchOrder.xlByRows, Excel.XlSearchDirection.xlNext, false,
                        oMissing, oMissing);
                }
            }
            catch (Exception ex)
            {
                ExcelFileException efe = new ExcelFileException(
                    "Exception in ExcelInterface::getRowNumberForSave(): ", ex);
                throw efe;
            }

            return currentRow;
        }

        void UpdateColumn(Excel.Range currentRow, string columnName, string newValue,
            List<string> columnNames)
        {
            int columnNumber = GetColumnNumber(columnName, columnNames);
            currentRow.Cells[1, columnNumber] = newValue;
        }

        // Get the apartment unit column for searching.
        private Excel.Range GetUnitColumn()
        {
            Excel.Range UnitColumn = null;

            try
            {
                string headerName = "UnitNo";
                Excel.Range headerRow = tenantRoster.UsedRange.Rows[1];

                foreach (Excel.Range cel in headerRow.Cells)
                {
                    if (cel.Text.ToString().Equals(headerName))
                    {
                        UnitColumn = tenantRoster.Range[cel.Address, cel.End[Excel.XlDirection.xlDown]];
                        return UnitColumn;
                    }
                }
            }
            catch (Exception ex)
            {
                ExcelFileException efe = new ExcelFileException(
                    "Exception in ExcelInterface::GetUnitColumn()!", ex);
                throw efe;
            }

            return UnitColumn;
        }

        private int GetColumnNumber(string columnName, List<string> columnNames)
        {
            int columnNumber = 1;

            foreach (string name in columnNames)
            {
                if (name.Equals(columnName))
                {
                    return columnNumber;
                }
                columnNumber++;
            }

            return columnNumber;
        }

        private List<string> GetColumnNames()
        {
            List<string> columnNames = new List<string>();

            try
            {
                Excel.Range headerRow = tenantRoster.UsedRange.Rows[1];
                foreach (Excel.Range cell in headerRow.Cells)
                {
                    columnNames.Add(cell.Text);
                }
            }
            catch (Exception ex)
            {
                ExcelFileException efe = new ExcelFileException(
                    "Exception in ExcelInterface::GetColumnNames()!", ex);
                throw efe;
            }

            return columnNames;
        }
    }
}

ExcelFileData.cs

using System;
using System.Collections.Generic;
using System.Data;

namespace TenantRosterAutomation
{
    public class ExcelFileData
    {
        private DataTable worksheetContents;
        private List<string> WorkSheets;

        public string ActiveWorkbookFullFileSpec { get; private set; }
        public string ActiveWorkSheet { get; private set; }
        public bool WorkbookRead { get; private set; }
        public bool WorkbookOpen { get; private set; }

        public ExcelFileData(string fullFileSpec, string activeWorkSheet)
        {
            if (string.IsNullOrEmpty(fullFileSpec))
            {
                ExcelFileException efe =
                    new ExcelFileException("Attempted to create ExcelFileData " +
                    "Object without Excel Data File Name.");
                throw efe;
            }
            ActiveWorkbookFullFileSpec = fullFileSpec;
            ActiveWorkSheet = activeWorkSheet;
            WorkSheets = null;
            worksheetContents = null;
            WorkbookOpen = false;
            WorkbookRead = false;
        }

        public void AddOrChangeWorkSheets(List<string> workSheets)
        {
            if (WorkSheets != null)
            {
                WorkSheets = null;
            }
            WorkSheets = workSheets;
        }

        public void ChangeActiveWorkbook(string fullFileSpec)
        {
            ActiveWorkbookFullFileSpec = null;
            ActiveWorkbookFullFileSpec = fullFileSpec;
        }

        public void ChangeActiveWorksheet(string newActiveWorkSheet)
        {
            ActiveWorkSheet = null;
            ActiveWorkSheet = newActiveWorkSheet;
        }

        public List<string> GetWorkSheetCollection()
        {
            if (WorkSheets == null)
            {

                using (ExcelInterface excelInterface = new ExcelInterface(
                    ActiveWorkbookFullFileSpec, ActiveWorkSheet))
                {
                    try
                    {
                        WorkSheets = excelInterface.GetWorkSheetNames();
                        excelInterface.Dispose();
                    }
                    catch (Exception)
                    {
                        excelInterface.Dispose();
                        throw;
                    }
                }
            }

            return WorkSheets;
        }

        public void SaveChanges(List<Apartment> tenantEdits)
        {
            if (tenantEdits.Count > 0)
            {
                if (string.IsNullOrEmpty(ActiveWorkSheet))
                {
                    ExcelFileException efe =
                        new ExcelFileException("Attempted to Save Excel changes " +
                        "without Excel worksheet name.");
                    throw efe;
                }

                using (ExcelInterface excelInterface = new ExcelInterface(
                    ActiveWorkbookFullFileSpec, ActiveWorkSheet))
                {
                    try
                    {
                        excelInterface.SaveEdits(tenantEdits);
                        excelInterface.Dispose();
                    }
                    catch (Exception)
                    {
                        excelInterface.Dispose();
                        throw;
                    }
                }
            }
        }

        public DataTable GetActiveWorkSheetContents(bool checkIfOPen)
        {
            if (worksheetContents == null)
            {
                if (string.IsNullOrEmpty(ActiveWorkSheet))
                {
                    ExcelFileException efe =
                        new ExcelFileException("Attempted to get Excel worksheet " +
                        "data without Excel worksheet name.");
                    throw efe;
                }
                using (ExcelInterface excelInterface = new ExcelInterface(
                    ActiveWorkbookFullFileSpec, ActiveWorkSheet))
                {
                    ReportCurrentStatusWindow statusReport = new ReportCurrentStatusWindow();
                    statusReport.MessageText =
                        "Starting Excel and Loading Tenant Data From Excel.";
                    statusReport.Show();
                    try
                    {
                        worksheetContents = excelInterface.GetWorkSheetContents();
                        excelInterface.Dispose();
                    }
                    catch (Exception)
                    {
                        statusReport.Close();
                        excelInterface.Dispose();
                        throw;
                    }

                    statusReport.Close();
                }
            }

            return worksheetContents;
        }
    }
}

TenantDataTable.cs

using System;
using System.Collections.Generic;
using System.Data;

namespace TenantRosterAutomation
{
    public class TenantDataTable
    {
        private bool dataChanged;
        private DataTable tenantData;
        // Updates are stored to a list to be written to back to the excel
        // file when the user clicks the save button in the UI or when the
        // user exits the program. Writing the entire DataTable back is
        // undesirable for 2 reasons, one is performance and the other is
        // to prevent corruption of the excel file.
        private List<Apartment> tenantUpdates;
        private ExcelFileData ExcelFile;

        public bool DataChanged { get { return dataChanged; } }
        public DataTable TenantRoster { get { return tenantData; } }

        public TenantDataTable(ExcelFileData excelFile)
        {
            ExcelFile = excelFile;
            if (string.IsNullOrEmpty(ExcelFile.ActiveWorkSheet))
            {

            }
            tenantData = excelFile.GetActiveWorkSheetContents(false);
            dataChanged = false;
            tenantUpdates = new List<Apartment>();
        }

        public Tenant GetTenant(int apartmentNumber)
        {
            Tenant tenant = null;
            try
            {
                if (tenantData != null)
                {
                    string searchString = "UnitNo = '" + apartmentNumber.ToString() + "'";
                    DataRow[] aptTenantData = tenantData.Select(searchString);
                    tenant = FillTenantFromDataRow(aptTenantData);
                }
            }
            catch (Exception e)
            {
                Exception Tdt = new Exception("Exception in TenantDataTable::GetTenant(): ", e);
                throw Tdt;
            }

            return tenant;
        }

        public bool SaveChanges()
        {
            bool successChange = true;
            if (dataChanged)
            {
                ExcelFile.SaveChanges(tenantUpdates);
                dataChanged = false;
                tenantUpdates.Clear();
            }

            return successChange;
        }

        public void DeleteTenant(int apartmentNumber)
        {
            Tenant tenant = new Tenant();
            tenantUpdates.Add(new Apartment(apartmentNumber, tenant));
            dataChanged = UdateTenantDataTable(apartmentNumber, tenant);
        }

        public void AddEditTenant(int apartmentNumber, Tenant tenant)
        {
            tenantUpdates.Add(new Apartment(apartmentNumber, tenant));
            dataChanged = UdateTenantDataTable(apartmentNumber, tenant);
        }

        // Creates data for a single tenant that can be edited from
        // the local data table.
        private Tenant FillTenantFromDataRow(DataRow[] aptTenantData)
        {
            Tenant tenant = new Tenant();

            tenant.LastName = aptTenantData[0].Field<string>("Last");
            tenant.FirstName = aptTenantData[0].Field<string>("First");
            tenant.CoTenantLastName = aptTenantData[0].Field<string>("Add OCC Last");
            tenant.HomePhone = aptTenantData[0].Field<string>("Ph #");
            tenant.CoTenantFirstName = aptTenantData[0].Field<string>("Add OCC First");
            tenant.RentersInsurancePolicy = aptTenantData[0].Field<string>("Renters Ins");
            tenant.LeaseStart = aptTenantData[0].Field<string>("Lease Start");
            tenant.LeaseEnd = aptTenantData[0].Field<string>("Lease End");
            tenant.Email = aptTenantData[0].Field<string>("Email");

            return tenant;
        }

        // Updates the local version of the data in the application.
        private bool UdateTenantDataTable(int apartmentNumber, Tenant tenant)
        {
            bool updated = false;
            try
            {
                string searchString = "UnitNo = '" + apartmentNumber.ToString() + "'";
                DataRow[] aptTenantData = tenantData.Select(searchString);
                DataRow currentApartment = aptTenantData[0];
                currentApartment.BeginEdit();
                currentApartment["First"] = tenant.FirstName;
                currentApartment["Last"] = tenant.LastName;
                currentApartment["Add OCC Last"] = tenant.CoTenantLastName;
                currentApartment["Add OCC First"] = tenant.CoTenantFirstName;
                currentApartment["Ph #"] = tenant.HomePhone;
                currentApartment["Renters Ins"] = tenant.RentersInsurancePolicy;
                currentApartment["Lease Start"] = tenant.LeaseStart;
                currentApartment["Lease End"] = tenant.LeaseEnd;
                currentApartment["Email"] = tenant.Email;
                currentApartment.EndEdit();
                updated = true;
            }
            catch (Exception e)
            {
                Exception Tdt = new Exception(
                    "Exception in TenantDataTable::updateDataTable(): ", e);
                throw Tdt;
            }

            return updated;
        }
    }
}

CheckExcelWorkBookOpen.cs

using System;
using System.Runtime.InteropServices;
using Excel = Microsoft.Office.Interop.Excel;

namespace TenantRosterAutomation
{
    public class CheckExcelWorkBookOpen
    {
        // Check if there is any instance of excel open using the workbook.
        public static bool IsOpen(string workBook)
        {
            Excel.Application TestOnly = null;
            bool isOpened = true;
            // There are 2 possible exceptions here, GetActiveObject will throw
            // an exception if no instance of excel is running, and
            // workbooks.get_Item throws an exception if the sheetname isn't found.
            // Both of these exceptions indicate that the workbook isn't open.
            try
            {
                TestOnly = (Excel.Application)Marshal.GetActiveObject("Excel.Application");
                int lastSlash = workBook.LastIndexOf('\\');
                string fileNameOnly = workBook.Substring(lastSlash + 1);
                TestOnly.Workbooks.get_Item(fileNameOnly);
                TestOnly = null;
            }
            catch (Exception)
            {
                isOpened = false;
                if (TestOnly != null)
                {
                    TestOnly = null;
                }
            }
            return isOpened;
        }

        // Common error message to use when the excel file is op in another app.
        public string ReportOpen(bool atStartUp)
        {
            string alreadyOpen = "The excel workbook " +
                Globals.Preferences.ExcelWorkBookFullFileSpec +
                    " is alread open in another application. \n" +
                    "Please save your changes in the other application and close the " +
                    "workbook and then" +
                    (atStartUp ? " try this operation again." : " restart this application.");

            return alreadyOpen;
        }

        public void TestAndThrowIfOpen(string workBook, bool atStartUp)
        {
            if (IsOpen(workBook))
            {
                AlreadyOpenInExcelException alreadOpen =
                    new AlreadyOpenInExcelException(ReportOpen(atStartUp));
                throw alreadOpen;
            }
        }
    }
}

AlreadyOpenInExcelException.cs

using System;

namespace TenantRosterAutomation
{
    [Serializable]
    public class AlreadyOpenInExcelException : Exception
    {
        public AlreadyOpenInExcelException(string message)
            : base(message)
        { }

    }
}

ExcelFileException.cs

using System;

namespace TenantRosterAutomation
{
    [Serializable]
    class ExcelFileException : Exception
    {
        public ExcelFileException(string message, Exception innerException = null)
            : base(message, innerException)
        { }

    }
}

Globals.cs

namespace TenantRosterAutomation
{
    // Perhaps this class and file should be renamed to GlobalModels
    // or just Modles. For performance reasons the classes provided
    // by this static class are created once at the beginning of the
    // program execution and deleted at the end of program execution.
    public static class Globals
    {
        private static string preferencesFileName = "./MyPersonalRentRosterPreferences.txt";
        public static TenantDataTable TenantRoster;
        public static PropertyComplex Complex;
        public static UserPreferences Preferences;
        public static ExcelFileData ExcelFile;

        public static bool InitializeAllModels()
        {
            bool everthingInitialized = false;
            ReleaseAllModels();

            Preferences = new UserPreferences(preferencesFileName);
            everthingInitialized = AdvancedInitialization();

            return everthingInitialized;
        }

        // Restart with new preferences
        public static bool ReInitizeAllModels(UserPreferences preferences)
        {
            bool everthingInitialized = false;
            ReleaseAllModels(false);

            Preferences.CopyValues(preferences);
            everthingInitialized = AdvancedInitialization();

            return everthingInitialized;
        }

        public static void ReleaseAllModels(bool releasePrefernces = true)
        {
            Complex = null;
            ExcelFile = null;
            TenantRoster = null;
            if (releasePrefernces)
            {
                Preferences = null;
            }
        }

        public static void Save()
        {
            SavePreferences();
            SaveTenantData();
        }

        public static void SavePreferences()
        {
            if (Preferences != null)
            {
                Preferences.SavePreferencesToFile();
            }
        }

        public static void SaveTenantData()
        {
            if (TenantRoster != null && TenantRoster.DataChanged)
            {
                TenantRoster.SaveChanges();
            }
        }

        private static bool AdvancedInitialization()
        {
            bool everthingInitialized = false;

            if (!string.IsNullOrEmpty(Preferences.ExcelWorkBookFullFileSpec) &&
                !string.IsNullOrEmpty(Preferences.ExcelWorkSheetName))
            {
                ExcelFile = CreateExcelDataFile();
                if (ExcelFile != null)
                {
                    TenantRoster = new TenantDataTable(ExcelFile);
                    if (TenantRoster != null && TenantRoster.TenantRoster != null)
                    {
                        ConstructComplexAndReport(TenantRoster);
                    }
                    everthingInitialized = true;
                }
            }

            return everthingInitialized;
        }

        private static void ConstructComplexAndReport(TenantDataTable TenantRoster)
        {
            ReportCurrentStatusWindow statusReport = new ReportCurrentStatusWindow();
            statusReport.MessageText = "Constructing Apartment Complex Data.";
            statusReport.Show();
            Complex = new PropertyComplex("Anza Victoria Apartments, LLC",
                TenantRoster.TenantRoster);
            statusReport.Close();
        }

        private static ExcelFileData CreateExcelDataFile()
        {
            ExcelFileData excelFile = null;
            if (!string.IsNullOrEmpty(Preferences.ExcelWorkBookFullFileSpec))
            {
                CheckExcelWorkBookOpen testOpen = new CheckExcelWorkBookOpen();
                testOpen.TestAndThrowIfOpen(Preferences.ExcelWorkBookFullFileSpec, true);
                testOpen = null;
                excelFile = new ExcelFileData(Preferences.ExcelWorkBookFullFileSpec,
                    Preferences.ExcelWorkSheetName);
            }

            return excelFile;
        }
    }
}

PropertyComplex.cs

using System;
using System.Collections.Generic;
using System.Data;
using System.Windows;

namespace TenantRosterAutomation
{
    // Models the entire apartment complex (5 buildings 177 apartmens)
    public class PropertyComplex
    {
        private string propertyName;
        private List<int> allApartmentNumbers;
        private List<Building> buildingList = new List<Building>();

        public string PropertyName { get { return propertyName; } }
        public List<Building> Buildings { get; private set; }
        public List<string> BuildingAddressList { get; private set; }
        public List<int> StreetNumbers { get; private set; }
        public List<int> AllApartmentNumbers { get { return allApartmentNumbers; } }
        public int MinApartmentNumber { get; private set; }
        public int MaxApartmentNumber { get; private set; }

        public enum ApartmentNumberValid
        {
            APARTMENT_NUMBER_OUT_OF_RANGE,
            APARTMENT_NUMBER_NONNUMERIC,
            APARTMENT_NUMBER_NOT_FOUND,
            APARTMENT_NUMBER_VALID
        }

        public PropertyComplex(string PropertyName, DataTable tenantRoster)
        {
            propertyName = PropertyName;
            BuildingAddressList = new List<string>();
            allApartmentNumbers = new List<int>();
            StreetNumbers = new List<int>();
            List<BuildingAndApartment> bldsAntApts =
                CreateBuildingAndApartmentsList(tenantRoster);
            CreateBuildingList(bldsAntApts);

            foreach (Building building in Buildings)
            {
                BuildingAddressList.Add(building.FullStreetAddress);
                allApartmentNumbers.AddRange(building.ApartmentNumbers);
            }

            allApartmentNumbers.Sort();

            MinApartmentNumber = allApartmentNumbers[0];
            MaxApartmentNumber = allApartmentNumbers[allApartmentNumbers.Count - 1];
        }

        public Building GetBuilding(int streetNumber)
        {
            Building building;

            building = buildingList.Find(x => x.AddressStreetNumber == streetNumber);

            return building;
        }

        public Building GetBuilding(string streetNumber)
        {
            int iStreetNumber = 0;

            try
            {
                if (Int32.TryParse(streetNumber, out iStreetNumber))
                {
                    return GetBuilding(iStreetNumber);
                }
                else
                {
                    MessageBox.Show("Non Numeric string passed into Building::GetBuilding().");
                    return null;
                }
            }
            catch (Exception)
            {
                return null;
            }
        }

        public string FindBuildingByApartment(int apartmentNumber)
        {
            string buildingAddress = null;

            foreach (Building building in Buildings)
            {
                buildingAddress = building.BuildingFromApartment(apartmentNumber);
                if (!string.IsNullOrEmpty(buildingAddress))
                {
                    return buildingAddress;
                }
            }

            return buildingAddress;
        }

        public ApartmentNumberValid VerifyApartmentNumber(string aptNumberString, out int apartmentNumber)
        {
            ApartmentNumberValid exitStatus = ApartmentNumberValid.APARTMENT_NUMBER_VALID;

            int aptNumber = 0;
            apartmentNumber = aptNumber;

            if (string.IsNullOrEmpty(aptNumberString))
            {
                return ApartmentNumberValid.APARTMENT_NUMBER_NONNUMERIC;
            }

            bool IsNumeric = int.TryParse(aptNumberString, out aptNumber);
            if (!IsNumeric)
            {
                return ApartmentNumberValid.APARTMENT_NUMBER_NONNUMERIC;
            }

            // 1/9/2022 Bugfix, some error messages contained the wrong value
            // for the apartment number.
            apartmentNumber = aptNumber;

            if (aptNumber < MinApartmentNumber || aptNumber > MaxApartmentNumber)
            {
                return ApartmentNumberValid.APARTMENT_NUMBER_OUT_OF_RANGE;
            }

            int found = 0;
            found = AllApartmentNumbers.Find(x => x == aptNumber);
            if (found == 0)
            {
                return ApartmentNumberValid.APARTMENT_NUMBER_NOT_FOUND;
            }

            apartmentNumber = aptNumber;
            return exitStatus;
        }

        public MailboxData GetMailBoxList(Building building)
        {
            MailboxData mailboxData = new MailboxData(building);
            List<int> apartmentNumbers = building.ApartmentNumbers;
            foreach (int aptNo in apartmentNumbers)
            {
                mailboxData.addApartmentData(new Apartment(aptNo));
            }

            return mailboxData;
        }

        private void CreateBuildingList(List<BuildingAndApartment> buildingAptList)
        {
            buildingList = new List<Building>();
            string streetName = "Anza Avenue";

            foreach (BuildingAndApartment entry in buildingAptList)
            {
                Building found = buildingList.Find(x => x.AddressStreetNumber == entry.building);
                if (found != null)
                {
                    found.AddApartmentNumber(entry.apartment);
                }
                else
                {
                    Building newBuilding = new Building(entry.building, streetName);
                    newBuilding.AddApartmentNumber(entry.apartment);
                    buildingList.Add(newBuilding);
                }
            }

            foreach (Building building in buildingList)
            {
                building.SortApartMentNumbers();
            }

            foreach (Building building in buildingList)
            {
                StreetNumbers.Add(building.AddressStreetNumber);
            }

            Buildings = buildingList;
        }

        private List<BuildingAndApartment> CreateBuildingAndApartmentsList(DataTable tenantRoster)
        {
            if (tenantRoster == null)
            {
                return null;
            }
            List<BuildingAndApartment> buildingAndApartments = new List<BuildingAndApartment>();
            int LastDataRow = tenantRoster.Rows.Count;

            for (int row = 0; row < LastDataRow; row++)
            {
                DataRow dataRow = tenantRoster.Rows[row];
                buildingAndApartments.Add(CreateBuildAndApartmentFromDataRow(dataRow));
            }

            return buildingAndApartments;
        }

        private BuildingAndApartment CreateBuildAndApartmentFromDataRow(DataRow dataRow)
        {
            string streetAddress = dataRow.Field<string>("Street 1").ToString();
            string apartmentNumString = dataRow.Field<string>("UnitNo").ToString();

            int apartmentNumber;
            Int32.TryParse(apartmentNumString, out apartmentNumber);

            int firstSpace = streetAddress.IndexOf(' ');
            string streetNumber = streetAddress.Substring(0, firstSpace);
            int buildingNumber;
            Int32.TryParse(streetNumber, out buildingNumber);

            BuildingAndApartment currentApt = new BuildingAndApartment(buildingNumber,
                apartmentNumber, streetAddress);
            return currentApt;
        }
    }
}

UserPreferences.cs

using System;
using System.Collections.Generic;
using System.IO;
using System.Windows.Forms;

namespace TenantRosterAutomation
{
    // Reads and writes the user preferences file. Stores the user
    // preferences for access to the excel spreadsheet as well as
    // storing print options.
    public class UserPreferences
    {
        private PrintSavePreference.PrintSave printSaveValue;
        private PrintSavePreference printSavePreference;
        private Dictionary<int, string> FieldNameByIndex;
        private Dictionary<string, int> IndexFromFieldName;
        private const int fileVersion = 1;
        private bool preferenceFileExists;
        private bool preferenceFileRead;
        private string preferencesFileName;
        private string workBookFullFileSpec;
        private string workSheetName;
        private string defaultSaveFolder;

        public bool HavePreferenceData { get { return preferenceFileRead; } }
        public PrintSavePreference.PrintSave PrintSaveOptions
        {
            get => printSaveValue;
            set { SetPrintSave(value); }
        }
        public string ExcelWorkBookFullFileSpec
        {
            get => workBookFullFileSpec; 
            set { SetExcelWorkBookFullFileSpec(value); }
        }
        public string ExcelWorkSheetName
        {
            get => workSheetName;
            set { SetExcelWorkSheetName(value); }
        }
        public string DefaultSaveDirectory
        {
            get => defaultSaveFolder;
            set { SetDefaultSaveDirectory(value); }
        }
        public bool PreferenceValuesChanged { get; private set; }

        public UserPreferences()
        {
            CommonInitialization();
        }

        public UserPreferences(string PreferencesFileName)
        {
            CommonInitialization();
            preferencesFileName = PreferencesFileName;

            if (!string.IsNullOrEmpty(preferencesFileName))
            {
                preferenceFileExists = File.Exists(preferencesFileName);
                if (preferenceFileExists)
                {
                    preferenceFileRead = ReadPreferenceFile(preferencesFileName);
                }
            }
        }

        public UserPreferences(UserPreferences original)
        {
            CommonInitialization();
            CopyValues(original, true);
        }

        public bool SavePreferencesToFile(string PreferencesFileName = null)
        {
            if (!PreferenceValuesChanged && !string.IsNullOrEmpty(PreferencesFileName))
            {
                return true;
            }

            if (!string.IsNullOrEmpty(PreferencesFileName))
            {
                preferencesFileName = PreferencesFileName;
            }

            StreamWriter Preferencesfile = new StreamWriter(preferencesFileName);
            try
            {
                WritePreferencesToDisc(Preferencesfile);
                Preferencesfile.Flush();
                Preferencesfile.Close();

                preferenceFileExists = true;
                preferenceFileRead = true;

                return true;
            }
            catch (IOException)
            {
                MessageBox.Show("Unable to write to preferences file: " + preferencesFileName);
                return false;
            }
        }

        public void CopyValues(UserPreferences newPreferences, bool copyPreferenceFileName = false)
        {
            PrintSaveOptions = newPreferences.PrintSaveOptions;
            ExcelWorkBookFullFileSpec = newPreferences.ExcelWorkBookFullFileSpec;
            ExcelWorkSheetName = newPreferences.ExcelWorkSheetName;
            DefaultSaveDirectory = newPreferences.DefaultSaveDirectory;
            if (copyPreferenceFileName)
            {
                preferencesFileName = newPreferences.preferencesFileName;
            }
        }

        private void SetExcelWorkBookFullFileSpec(string newName)
        {
            workBookFullFileSpec = newName;
            PreferenceValuesChanged = true;
        }

        private void SetExcelWorkSheetName(string newName)
        {
            workSheetName = newName;
            PreferenceValuesChanged = true;
        }

        private void SetDefaultSaveDirectory(string newName)
        {
            defaultSaveFolder = newName;
            PreferenceValuesChanged = true;
        }

        private void SetPrintSave(PrintSavePreference.PrintSave PrintSave)
        {
            printSaveValue = PrintSave;
            PreferenceValuesChanged = true;
        }

        private void CommonInitialization()
        {
            preferencesFileName = null;
            preferenceFileExists = false;
            preferenceFileRead = false;
            printSavePreference = new PrintSavePreference();
            InitDictionaries();
            SetValuesToUndefinedState();
        }

        private void InitDictionaries()
        {

            FieldNameByIndex = new Dictionary<int, string>();
            int fieldNameCount = fileValueIds.Length;
            for (int i = 0; i < fieldNameCount; ++i)
            {
                FieldNameByIndex.Add(i, fileValueIds[i]);
            }

            IndexFromFieldName = new Dictionary<string, int>();
            for (int i = 0; i < fieldNameCount; ++i)
            {
                IndexFromFieldName.Add(fileValueIds[i], i);
            }

        }

        private void SetValuesToUndefinedState()
        {
            PrintSaveOptions = PrintSavePreference.PrintSave.PrintOnly;
        }

        // These constants and variables are used when reading and writing
        // the preferences to and from the preference file.
        private const int fileVersionId = 0;
        private const int printSaveOptionId = 1;
        private const int defaultSaveDirId = 2;
        private const int rentRosterFileId = 3;
        private const int rentRosterSheetNameId = 4;
        private string[] fileValueIds =
        {
            "FileVersion:",
            "PrintSaveValue:",
            "DefaultSaveDirectory:",
            "RentRosterFile:",
            "RentRosterSheet:"
        };

        private void WritePreferencesToDisc(StreamWriter preferenceFile)
        {
            preferenceFile.WriteLine(fileValueIds[fileVersionId] + " " + fileVersion.ToString());
            preferenceFile.WriteLine(fileValueIds[printSaveOptionId] + " " +
                printSavePreference.ConvertPrintSaveToString(printSaveValue));
            preferenceFile.WriteLine(fileValueIds[defaultSaveDirId] + " " + DefaultSaveDirectory);
            preferenceFile.WriteLine(fileValueIds[rentRosterFileId] + " " + ExcelWorkBookFullFileSpec);
            preferenceFile.WriteLine(fileValueIds[rentRosterSheetNameId] + " " + ExcelWorkSheetName);

        }

        private void ConvertandTestFileVersion(string fileInput)
        {
            try
            {
                int testFileVersion = Int32.Parse(fileInput);
                if (testFileVersion != fileVersion)
                {
                    if (testFileVersion < fileVersion)
                    {
                        MessageBox.Show("Preference file version " + fileInput + " out of date, please edit preferences to add new field values.");
                    }
                    else
                    {
                        MessageBox.Show("This version of the Tenant Roster tool does not support all the features of the tool that generated the file.");
                    }
                }
            }
            catch (FormatException e)
            {
                string eMsg = "Reading preferences File Version failed: " + e.Message;
                MessageBox.Show(eMsg);
            }
        }

        private bool ReadPreferenceFile(string fileName)
        {
            bool fileReadSucceeded = true;
            string[] lines;

            if (File.Exists(fileName))
            {
                try
                {
                    lines = File.ReadAllLines(fileName);
                    fileReadSucceeded = GetPreferenceValues(lines);
                }
                catch (IOException)
                {
                    fileReadSucceeded = false;
                }
            }

            return fileReadSucceeded;
        }

        private bool GetPreferenceValues(string[] lines)
        {
            bool hasAllFields = false;
            int requiredFieldCount = 0;
            int lineCount = lines.Length;
            for (int i = 0; i < lineCount; ++i)
            {
                string[] nameAndValue = lines[i].Split(' ');
                // The first value in the line is the field index, if there
                // is no second value then the preference is not specified
                if (nameAndValue.Length < 2)
                {
                    return false;
                }

                int fieldIndex;
                IndexFromFieldName.TryGetValue(nameAndValue[0], out fieldIndex);
                switch (fieldIndex)
                {
                    case fileVersionId:
                        ConvertandTestFileVersion(nameAndValue[1]);
                        requiredFieldCount++;
                        break;

                    case printSaveOptionId:
                        printSaveValue = printSavePreference.ConvertStringToPrintSave(nameAndValue[1]);
                        requiredFieldCount++;
                        break;

                    case defaultSaveDirId:
                        DefaultSaveDirectory = CorrectForMuliWordNames(nameAndValue);
                        requiredFieldCount++;
                        break;

                    case rentRosterFileId:
                        ExcelWorkBookFullFileSpec = CorrectForMuliWordNames(nameAndValue);
                        requiredFieldCount++;
                        break;

                    case rentRosterSheetNameId:
                        ExcelWorkSheetName = CorrectForMuliWordNames(nameAndValue);
                        requiredFieldCount++;
                        break;

                    default:
                        MessageBox.Show("Reading preference file: Unknown field identity");
                        return false;
                }
            }

            if (requiredFieldCount == fileValueIds.Length)
            {
                hasAllFields = true;
                if (string.IsNullOrEmpty(ExcelWorkSheetName))
                {
                    hasAllFields = false;
                }
            }

            return hasAllFields;
        }

        private string CorrectForMuliWordNames(string[] lineValues)
        {
            // The first value in the line is the field index, if there
            // is no second value then the preference is not specified
            if (lineValues.Length < 2)
            {
                return "";
            }

            string wholeName = lineValues[1];
            for (int i = 2; i < lineValues.Length; i++)
            {
                wholeName += " " + lineValues[i];
            }

            return wholeName;
        }

    }
}

Program.cs

using System;
using System.Windows.Forms;

namespace TenantRosterAutomation
{
    static class Program
    {
#if DEBUG
        [DllImport("kernel32.dll")]
        static extern bool AttachConsole(int dwProcessId);
        private const int ATTACH_PARENT_PROCESS = -1;
#endif

        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        [STAThread]
        static void Main()
        {
#if DEBUG
            AttachConsole(ATTACH_PARENT_PROCESS);
            Console.WriteLine("\nStarting TenantRosterAutomation Application");
#endif
            try
            {
                Application.EnableVisualStyles();
                Application.SetCompatibleTextRenderingDefault(false);
                Application.Run(new RentRosterApp());
            }
            catch (AlreadyOpenInExcelException e)
            {
                MessageBox.Show(e.Message);
            }
            catch (Exception e)
            {
                MessageBox.Show("An unexpected error occurred: " + e.ToString());
            }
        }
    }
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ @t3chb0t I would like a follow up review. \$\endgroup\$
    – pacmaninbw
    Commented Jan 13, 2022 at 22:03
  • \$\begingroup\$ @AlanT Any chance you would like to do a follow up review. \$\endgroup\$
    – pacmaninbw
    Commented Jan 13, 2022 at 22:04
  • 1
    \$\begingroup\$ That's weird. I've just found you mentioning me accidentally as there is no notification in my inbox :o I'll see what I can do :-] \$\endgroup\$
    – t3chb0t
    Commented Jan 14, 2022 at 7:38
  • \$\begingroup\$ @t3chb0t I'm going to assume by the lack of answer that I covered most or all of the issues your original review with the exception of the ``Verbose code` versus var, which I explained in a comment in the original question. I'm going to wait another day before I accept one of the existing answers. Feel free to post issues in my GitHub Repository also. \$\endgroup\$
    – pacmaninbw
    Commented Jan 16, 2022 at 15:25
  • \$\begingroup\$ I'd rather assume it was a busy week and I didn't have time to look at it ;-] \$\endgroup\$
    – t3chb0t
    Commented Jan 17, 2022 at 7:40

3 Answers 3

1
\$\begingroup\$

I'm happy to see that we were able to convince you to use the C# naming convension :-]

Here are my other thoughs...

ExcelInterface

To my taste the ExcelInterface.SaveEdits method either does too much for a Save method or it shouldn't be called like that. Internally it calls UpdateColumnData and this is the part that bothers me. A clean Save shouldn't be doing things like that. I would not expect it to change my workseet.


I don't think you should call StartExcelOpenWorkbook everywhere. I my opinion the instance of ExcelInterface should be fully initialized at creation time. Otherwise it's too easy to implement something new and to forget that it's necessary to call this method first.


Throwing exceptions that use the class and method names as a message like Function ExcelInterface.OpenTenantRosterWorkSheet() failed! doesn't have much value as the stack-strace already provides the same information. It'd be much more helpful to the user if you gave them other details like the name of the excel-file or the worksheet or the row or some other data that is relevant to an exception and provides more context.


Try to write pure functions where possible. One such candidate is the OpenTenantRosterWorkSheet method. Currently it does something and sets the tenantRoster field. This side-effect is unexpected and you need to know its implementation in order understand its purpose.

I'd start with something like this. Where you explicitly pass the necessary parameters and explictly get a result. It might look like an overkill at first but when you keep writing code like this it might quickly turn out that a method is actually so gneric that it could be extracted into a utility method like an extension.

The next step would be to rewrite GetWorkSheetNames. It's almost there, it just needs to take some more parameters and you can turn it into static.

This way you'd greatly reduce the complexity and made the code more intuitive to use.

        private Excel.Worksheet? OpenWorksheetOrDefault(Excel.Workbook xlWorkbook, string tenantRosterName)
        {
            try
            {
                if (!string.IsNullOrEmpty(tenantRosterName))
                {
                    List<string> sheetNames = GetWorkSheetNames();
                    bool exists = sheetNames.Any(x => x.Contains(tenantRosterName));
                    if (!exists)
                    {
                        MessageBox.Show("The workbook " + WorkbookName + " does not contain the worksheet " + tenantRosterName);
                        return default;
                    }
                    else
                    {
                        return xlWorkbook.Worksheets[tenantRosterName];
                    }
                }
            }
            catch (Exception ex)
            {
                string eMsg = "Function ExcelInterface.OpenTenantRosterWorkSheet() failed! ";
                ExcelFileException efe = new ExcelFileException(eMsg, ex);
                throw efe;
            }
        }

GetUnitColumn could use some more forgiving string conditions. Currently it's case sensitive and doesn't trim strings. From my experience I know this easily goes sideways. I even use my own custom SoftString class to avoid the case and white-space trap.


Dispose(bool disposing) could be simplified with the null propagation operator like this. No ifs requried:

xlWorkbook?.Close();
xlWorkbook = null;

xlApp?.Quit();
xlApp = null;

// Ensure that Excel exits after this class is disposed.
// In some cases orphaned Excel proceses remain.
Process.GetProcessById(ExcelProcessId)?.Kill();

ExcelFileData

        public void AddOrChangeWorkSheets(List<string> workSheets)
        {
            if (WorkSheets != null)
            {
                WorkSheets = null;
            }
            WorkSheets = workSheets;
        }

This, ChangeActiveWorkbook, and ChangeActiveWorksheet could be properties.


TenantDataTable

        public TenantDataTable(ExcelFileData excelFile)
        {
            ExcelFile = excelFile;
            if (string.IsNullOrEmpty(ExcelFile.ActiveWorkSheet))
            {

            }
            tenantData = excelFile.GetActiveWorkSheetContents(false);
            dataChanged = false;
            tenantUpdates = new List<Apartment>();
        }

I like that if condition. It looks really useful ;-P


string searchString = "UnitNo = '" + apartmentNumber.ToString() + "'";

This could be replaced with string interpolation:

var searchString = $"UnitNo = '{apartmentNumber}'";

UdateTenantDataTable can either return bool or throws an exception if it fails. It should be either or. Both mechanisms at the same time are redundant as it can never return false.


CheckExcelWorkBookOpen

IsOpen works with the workbook name and extracts the file-name:

int lastSlash = workBook.LastIndexOf('\\');
string fileNameOnly = workBook.Substring(lastSlash + 1);

This can be done easier with the Path class that provides a couple of handy methods to work with paths and file names.

\$\endgroup\$
5
  • \$\begingroup\$ Ecellent review! I need to look at the Path Class! The string interpolation looks really useful as well (for real). Re: C Naming Conventions, I don't even name my C++ classes that way. I needed a way to differentiate modules versus forms. The sub directories helped, I may even break up classes more, moving all the exception classes into their own directory (I've added 2 more exception classes since I posted this). If you look in the repository you will see that AddOrChangeWorkSheets and that really useful if statement have been addressed. :) \$\endgroup\$
    – pacmaninbw
    Commented Jan 17, 2022 at 19:28
  • \$\begingroup\$ Question Re: To my taste the ExcelInterface.SaveEdits method either does too much for a Save method or it shouldn't be called like that. Internally it calls UpdateColumnData and this is the part that bothers me. A clean Save shouldn't be doing things like that. I would not expect it to change my workseet. Are you saying that the should be another interface function who's sole purpose is to update the edits and that this function should just call xlWorkbook.save()? The current model is to open the workbook access the worksheet if necessary and then close the workbook and quit. \$\endgroup\$
    – pacmaninbw
    Commented Jan 18, 2022 at 22:00
  • 1
    \$\begingroup\$ 10 new issues in my repository issues list two of which have been already closed because the work was already done. My question above has been added to the relevant issue as a comment. Thank you very much! \$\endgroup\$
    – pacmaninbw
    Commented Jan 19, 2022 at 0:13
  • 1
    \$\begingroup\$ @pacmaninbw yes, I think it would be better to save changes on edit as this is usually what you expect but not really that anything changes when you save something. You've really created issues for it, wow :o If all projects looked like that heh I rarely have enough discipline to do this :P \$\endgroup\$
    – t3chb0t
    Commented Jan 19, 2022 at 20:33
  • \$\begingroup\$ I don't know if you are familiar with the Capability Maturity Model for Software from the Software Engineering Institute at Carnegie Mellon University, but in 2001 I was certified as an instructor. I didn't keep up the certification for it because it costs $10,000 USD a year, and Citigroup didn't want to pay for it, I also had problems getting enough hours to keep it going, since Citigroup moved away from it. Teradyne, another company I worked for got me started on CMM. \$\endgroup\$
    – pacmaninbw
    Commented Jan 19, 2022 at 22:40
2
\$\begingroup\$

UI Dependencies in the Services
Removing most (why not all?) of the MessageBox() calls is good but calling the ReportCurrentStatusWindow() from within SaveEdits() gives us the same problem. A dependency on a (possibly changing) UI layer element for our service. We may never want to use this service in a console app or web service but adding the window means that we cannot and it makes unit testing problematic.

If we do want to leave it in, the using a finally clause will remove repeated code

public void SaveEdits(List<Apartment> tenantUpdates)
{
    ReportCurrentStatusWindow SaveStatus = new ReportCurrentStatusWindow();
    string eSaveMsg = "Can't save edits to " + WorkbookName;
    try
    {
        SaveStatus.MessageText = "Saving updated tenants and apartments to Excel.";
        SaveStatus.Show();
        StartExcelOpenWorkbook();
        OpenTenantRosterWorkSheet();
        xlApp.Visible = false;
        xlApp.DisplayAlerts = false;

        if (tenantRoster == null)
        {
            MessageBox.Show(eSaveMsg + " can't open the excel worksheet "
                    + tenantRosterName + " to save changes");
            return;
        }
        List<string> columnNames = GetColumnNames();
        foreach (Apartment edit in tenantUpdates)
        {
            UpdateColumnData(edit, columnNames);
        }
        xlWorkbook.Save();
        
    }
    catch (AlreadyOpenInExcelException)
    {
        throw;
    }
    catch (Exception ex)
    {
        ExcelFileException efe = new ExcelFileException(eSaveMsg, ex);
        throw efe;
    }
    finally
    {
        SaveStatus.Close();
    }
}

NOTE: In the current code, if the tenantRoster doesn't load (i.e. == null) then we do not call SaveStatus.Close(). Is this the intent?

Disposing
There is no need to call Dispose() explicitly inside a using clause. The purpose of the using clause is to ensure that Dispose is called

public List<string> GetWorkSheetCollection()
{
    if (WorkSheets == null)
    {
        using (ExcelInterface excelInterface = new ExcelInterface(
                ActiveWorkbookFullFileSpec, ActiveWorkSheet))
        {
            try
            {
                WorkSheets = excelInterface.GetWorkSheetNames();
                excelInterface.Dispose();  // NOT NEEDED
             }
             catch (Exception)
             {
                    excelInterface.Dispose(); // NOT NEEDED
                    throw;
             }
        }
    }

    return WorkSheets;
}

Possibly related, but setting an object to null before assigning it is not required or useful. Was it intended to try and force the previous list to be garbage collected?

 public void AddOrChangeWorkSheets(List<string> workSheets)
 {
     if (WorkSheets != null)
     {
         WorkSheets = null;
     }
     WorkSheets = workSheets;
 }

Similarly not useful for string values

public void ChangeActiveWorkbook(string fullFileSpec)
{
    ActiveWorkbookFullFileSpec = null;
    ActiveWorkbookFullFileSpec = fullFileSpec;
}

public void ChangeActiveWorksheet(string newActiveWorkSheet)
{
    ActiveWorkSheet = null;
    ActiveWorkSheet = newActiveWorkSheet;
}

Process Killing
This probably comes under a Matter of Personal Preference (MoPP (tm)) but using the process killer seems like a bad idea. If we have good error handling in place, is shouldn't be needed. The orphans are usually a result of earlier problems which should be addressed there (e.g. with finally clauses)

Also, AFAIK setting the references to null doesn't really help us. A better solution might to be to use if we want to explicitly release the RCWs

\$\endgroup\$
6
  • \$\begingroup\$ I'll look at the 2 references provided about releasing the RCWs. I spent over a day trying to get rid of orphan excel processes and settled on killing them. The explicit call to Dispose was added during this debug session as well, I thought the end of the using statement would handle the dispose. I discovered the use of the ReportCurrentStatusWindow in SaveEdits() this morning while cleaning up unused functions and unnecessary vertical spacing, much to my chagrin, good catch. FYI - setting xlApp.Visible code is duplication of code as well. \$\endgroup\$
    – pacmaninbw
    Commented Jan 14, 2022 at 16:48
  • \$\begingroup\$ Did I capture all the issues correctly in GitHub? \$\endgroup\$
    – pacmaninbw
    Commented Jan 14, 2022 at 17:39
  • \$\begingroup\$ I've left the process killing in for the time being. I removed all message forms of any type from the services. I have removed the explicit call to Dispose from the try block of all the using statements, I have left them in the catch blocks because I'm not sure about the semantics with a throw inside a using statement. I have closed all the issues I opened earlier. You can find the changed code in the GitHub repository. \$\endgroup\$
    – pacmaninbw
    Commented Jan 15, 2022 at 0:42
  • \$\begingroup\$ FYI - your suggestion for me to move all user messaging to the UI helped me find possible bugs in the UI. Specifically there were problems in RentRosterApp.cs when using the standard windows close. I have changed all forms by removing the control box and exiting by clicking a button. \$\endgroup\$
    – pacmaninbw
    Commented Jan 15, 2022 at 17:27
  • 1
    \$\begingroup\$ @pacmaninbw, yes, it is not neccessarily immediately but the point was that it does something more than simply setting the reference to null. We are explicitly telling the COM object that we are finished with it and that it can go away now. We do depend upon the COM object's implementation but I have found with that with Excel calling this when I have finished with an interop COM object has helped solve problems with zombie/orphan Excel instances. \$\endgroup\$
    – AlanT
    Commented Jan 17, 2022 at 9:49
1
\$\begingroup\$

I once faced the orphan Excel process issue. I eventually got it resolved using code like this for your ExcelInterface.cs:

public void Dispose()
{
    Dispose(true);

    // Wait for Excel COM object to cleanup
    // https://stackoverflow.com/questions/10309365/the-proper-way-to-dispose-excel-com-object-using-vb-net
    GC.Collect;
    GC.WaitForPendingFinalizers();
    GC.Collect;
    GC.WaitForPendingFinalizers();
}

I don't understand the design decision to test if the Excel worokbook is open and throw an exception if it is. I use two different methods for returning open objects if I want to reusing existing sessions.

public void TryGetExcelApp()
{
    try
    {
        xlApp = Marshal.GetActiveObject("Excel.Application");
    }
    catch (Exception ex)
    {
        Debug.WriteLine("Excel isn't open, opening a new instance...");
    }

    if (xlApp = null)
    {
        xlApp = new Excel.Application()
    }
}

If your goal is to avoid collision with an existing open file then you should change the name of the file every time you open it and work on that temporary file, leaving the original unchanged.

Try\Throw

It looks like every time you perform a try you throw the error again. I don't understand the reason for doing so and it's an anti-pattern. If you want the exceptions to bubble up, don't use try at all.

\$\endgroup\$
4
  • \$\begingroup\$ Just curious, how do you go about merging the changes between the two excel files when you save it as a separate file? In this case the excel file is the master data, if it is open in excel then the application really shouldn't open it. If this was a database such as MS SQL Server, MySQL or MariaDB this wouldn't be a problem at all because locking data is automatic. I'm not trying to open a previous session I'm trying to prevent data corruption. \$\endgroup\$
    – pacmaninbw
    Commented Jan 14, 2022 at 20:10
  • \$\begingroup\$ It really depends on what you are doing to the data. If this is a multi-user system then checking if the file is open locally won't save you. You might want to consider using an AccessDB as the backend linked to an Excel table. This would make the Excel doc a read-only view. You could have a different process for importing data and editing in Access. You can then do a MailMerge from access to generate the word documents magically. \$\endgroup\$
    – HackSlash
    Commented Jan 14, 2022 at 21:02
  • \$\begingroup\$ I just realized that you might be missing some background about this question. I originally wrote this code in VBA for excel and that would have solved any data consistency problems. The problem with the VBA solution is that the version of excel installed on the users computer doesn't support VBA. When I got a closer look at the excel spreadsheet I noticed it already had macros, so the original version probably did a lot of this. \$\endgroup\$
    – pacmaninbw
    Commented Jan 15, 2022 at 14:14
  • \$\begingroup\$ The current usage is to open the document and edit the rows in the table, there are currently fields in a row that this solution doesn't address, but the tool handles all the every day tasks. It is possible that the user will need to edit something directly in excel and forget to close it or save the changes from the tool first. \$\endgroup\$
    – pacmaninbw
    Commented Jan 15, 2022 at 14:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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