3
\$\begingroup\$

I've got an Excel file from which I have to read out data to an object to serialize. So far I came up with this solution, and I'm curious if there are any clearer solutions.

public myClassFromXsd excelToMyClassFromXsd(String excelFile)
            {
                Excel.Application excelApp = new Excel.Application();            
                Excel.Workbook workBook = excelApp.Workbooks.Open(excelFile);
                Excel.Worksheet workSheet = (Excel.Worksheet)workBook.Sheets[1];

                myClassFromXsd spec = new myClassFromXsd();
                UserHeader user = new UserHeader();
                ResultHeader header = new ResultHeader();
                anotherClassFromXsd specOp = new anotherClassFromXsd();
                List<anotherClassFromXsd> specOpList = new List<anotherClassFromXsd>();
                thisIsAClassToo tcd = new thisIsAClassToo();            

                excelApp.Visible = false;            

                tcd.orderNumber = Convert.ToString(workSheet.get_Range("B3").Value); 
                switch (((String)workSheet.get_Range("B4").Value).ToUpper())
                {
                    case "I":
                        tcd.typeOfThis = EnumFromXsd.I;
                        break;
                    case "E":
                        tcd.typeOfThis = EnumFromXsd.E;
                        break;
                    case "D":
                        tcd.typeOfThis = EnumFromXsd.D;
                        break;
                    default:
                        break;
                }
                switch (((String)workSheet.get_Range("B5").Value).ToUpper())
                {
                    case "I":
                        tcd.othersCanAlter = true;
                        break;
                    case "N":
                        tcd.othersCanAlter = false;
                        break;                
                    default:
                        break;
                }
                tcd.oneProperty = Convert.ToString(workSheet.get_Range("B6").Value);
                tcd.onePropertyText = Convert.ToString(workSheet.get_Range("B7").Value);
                tcd.Name = Convert.ToString(workSheet.get_Range("B10").Value);
                tcd.vatNum = Convert.ToString(workSheet.get_Range("B11").Value);
                tcd.city = Convert.ToString(workSheet.get_Range("B12").Value);                  


                specOp.index = 1;
                specOp.operation = Operation.create;
                specOp.Item = tcd;
                specOpList.Add(specOp);    

                header.requestId = Guid.NewGuid().ToString(); 
                header.timestamp = DateTime.Now.ToString("yyyy-mm-ddTH:mm:sszzz");              


                user.user = "TestUser";
                user.passwordHash = "BA32521232AED23798735C78739273GDHS238238723";

                spec.header = header;
                spec.user = user;
                spec.specOps = specOpList;

                excelApp.Quit();
                return spec;
            }       

And the serializer and deserializer methods:

public void serializeXml(myClassFromXsd mcfx)
        {
            using (StreamWriter writer = new StreamWriter("e:\\test.xml", false))
            {

                XmlSerializer serializer = new XmlSerializer(typeof(myClassFromXsd));
                serializer.Serialize(writer, mcfx);
            }
        }

        public myClassFromXsd deserializeXml(String xmlFile)
        {
            myClassFromXsd mcfx = new myClassFromXsd();
            XmlSerializer serializer = new XmlSerializer(typeof(myClassFromXsd));
            StreamReader reader = new StreamReader(xmlFile);
            mcfx = (myClassFromXsd)serializer.Deserialize(reader);
            reader.Close();
            return mcfx;
        }
\$\endgroup\$
2
\$\begingroup\$

The indendation looks off - assuming it's not a paste glitch with tabs vs spaces, the scope-opening brace should line up with the method's signature:

public myClassFromXsd excelToMyClassFromXsd(String excelFile)
            {

Like this:

public myClassFromXsd excelToMyClassFromXsd(String excelFile)
{

Also, C# type names should be PascalCase, as well as any public members:

  • myClassFromXsd should be MyClassFromXsd
  • serializeXml should be SerializeXml
  • deserializeXml should be DeserializeXml

The get_Xxxx methods in the COM Interop interface aren't following this convention, but they're COM Interop methods, with their own conventions - they're not an example to follow for typical C# code.

I like that you're wrapping your StreamWriter in a using block. I don't understand why you're not doing the same with the StreamReader in the deserializeXml method - the reader.Close() call could then be removed:

    public myClassFromXsd deserializeXml(String xmlFile)
    {
        myClassFromXsd mcfx = new myClassFromXsd();
        XmlSerializer serializer = new XmlSerializer(typeof(myClassFromXsd));
        using (StreamReader reader = new StreamReader(xmlFile))
        {
            mcfx = (myClassFromXsd)serializer.Deserialize(reader);
        }
        return mcfx;
    }

I would give mcfx a meaningful name: as it stands it looks like it's just a shortened version of the return type's name - I like having a result for these kinds of things. Identifiers should have a pronounceable name that carries their meaning.

\$\endgroup\$
1
\$\begingroup\$

One thing to note is that anytime you're creating an instance of Excel, you need to wrap that code in a try...catch...finally. You take in a string file path, but what if it's not a valid file? Then this line

Excel.Workbook workBook = excelApp.Workbooks.Open(excelFile);

will throw an error and you're left with an invisible instance of Excel left hanging.


All of these variable types declarations could probably be replaced with the var keyword. I know the last one could.

Excel.Application excelApp = new Excel.Application();            
Excel.Workbook workBook = excelApp.Workbooks.Open(excelFile);
Excel.Worksheet workSheet = (Excel.Worksheet)workBook.Sheets[1];

myClassFromXsd tells me almost nothing about what that class is. It's enough to guess that's it's serialized from an xsd file, but that's about it. Even then, it's a name that tells me about implementation details instead of what the object represents.


Do yourself a favor and generalize your Serialize method a little bit. It should be generic and take in a file path as an argument.

public void serializeXml<T>(T toSerialize, string filePath)
{
    using (StreamWriter writer = new StreamWriter(filePath, false))
    {
        XmlSerializer serializer = new XmlSerializer(typeof(T));
        serializer.Serialize(writer, toSerialize);
    }
}

And then you could easily reuse that method serializing any object you ever may need to.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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