HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Short XML parser

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
parserxmlshort

Problem

This is my first code in C# ever. It compiles and works as intended (not complete), but I want to see what I'm doing right and wrong as a first-timer.

```
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Xml;
using System.Xml.Schema;
using System.Xml.Serialization;
using System.Xml.Linq;
using System.IO;

namespace WindowsFormsApplication2
{
public partial class Form1 : Form
{
public Form1()
{
InitializeComponent();
}

///
/// Event driven by browsing a file
///
///
///
private void uploadFile_Click(object sender, EventArgs e)
{
OpenFileDialog oOpenFileDialog = new OpenFileDialog();
oOpenFileDialog.Title = "Open XML File";
oOpenFileDialog.Filter = "XML Files|*.xml";
if (!oOpenFileDialog.CheckFileExists) { MessageBox.Show("Invalid Filename!"); }
if (IsLinux) { MessageBox.Show("Linux! Not supported!"); }
else { oOpenFileDialog.InitialDirectory = @"C:\...too..long"; }

if (oOpenFileDialog.ShowDialog() == DialogResult.OK)
{
buildTree(oOpenFileDialog.FileName.ToString());

PBC pbcXmlData = Deserialize(oOpenFileDialog.FileName.ToString());
Console.WriteLine("----------------------------------");
Console.Write("Validating.......");
ValidateXml(oOpenFileDialog.FileName.ToString());
Console.WriteLine();
Console.WriteLine("File Path: " + oOpenFileDialog.FileName.ToString());
PrintXmlData(pbcXmlData);
Console.WriteLine("-----------------------------------");
}
}

///
/// Builds the treeview of the user's XML file
///
/// XML Filename

Solution

Overall Impression

Applications should be split into layers.

I know it's a simple, throwaway, demo application but these are a perfect opportunity to start working on splitting an application into a Model, View and X. (A Matter Of Personal Preference (AMOPP); looking at the namespaces, you are working with WinForms - I would go with WPF instead. Have a google and see what you think of the arguments)

Naming

Hungarian Notation is not recommended for C# code.

Pascal Naming is recommended for methods (e.g. BuildTree() rather than buildTree() )(though this may just be a typo as the others are Pascal Cased)

PBC is not a useful class name. Something descriptive of the function is better. Similarly Elements, is too vague and general.

Other Things

I find

if (!oOpenFileDialog.CheckFileExists) { MessageBox.Show("Invalid Filename!"); }


puzzling. It has been a while but, as far a I remember, CheckFileExists is a property that one sets before showing the dialog, telling it to display a warning (or not) if the file selected doesn't exist. The code above is not the correct usage (and will never show the "Invalid FileName!" message because the default is true.)

The processing order seems off, you are deserializing the contents of the file (I'd pass in a stream here, rather than a file name. It gives much more flexibility), then validating it - which, AFAIK, should never run if the Xml is invalid because the deserialization will throw an exception (Haven't tested this so I may be wrong, but either way, validating after a successful load seems off).

Grab the file name out of the open dialog and store it so that you do not have to keep using oOpenFileDialog.FileName.ToString()

Code Snippets

if (!oOpenFileDialog.CheckFileExists) { MessageBox.Show("Invalid Filename!"); }

Context

StackExchange Code Review Q#56311, answer score: 5

Revisions (0)

No revisions yet.