C #: Design of gadgets (classes / interfaces, etc.)

Question

I am creating a small application that allows the user to convert images to various sizes and formats. I'm struggling to get a nice solid design with this app. I have an application and it works, but it integrates a good object oriented design. Since this is a personal project, I would like to learn more about interface integration, good class inheritance, object compilation, and other OO design elements.

However, I struggled to do this. Don't get me wrong, I know about OO design and what it is, I just don’t know how to implement good OO design in projects. Of course, it’s easy to look at examples of classes that you read in books or on the Internet. Examples may have simple scenarios, such as the following.

The IPerson interface has the member functions Walk (), Run (). The abstract face of the class uses the IPerson interface. Class A man and a class Women are inherited from the abstract class.

but when it comes to real projects , I try to implement a good design. I was hoping for some understanding. Here is what I have now.

Interface:

interface IPicture { Bitmap ReturnImage(string path, int width, int height); } 

The main class that contains information about the snapshot. This class basically stores information about the transferred image and information about the new values ​​that the user wants (i.e., New size, new file location, new pic format, etc.).

 public class MyPictures : IPicture { //All Private variables below are properties. Property get/set have been removed //for the sake of space private int _NewWidth; private int _NewHeight; private string _NewImgName; private string _NewImgPath; private string _NewImgFullPath; private ImageFormat _NewImgFormat; //Declare variables to hold values that have been determined private int _OldWidth; private int _OldHeight; private string _OldImgName; private string _OldImgPath; //Old Image Format is in String format because of certain extension scenarios. private string _OldImgFormat; public MyPictures(Image img, string file) { ClearProperties(); //...set properties based on passed variables in constructor... } public void ClearProperties() { _NewWidth = 0; _NewHeight = 0; _NewImgName = ""; _NewImgPath = ""; _NewImgFullPath = ""; _NewImgFormat = null; _OldWidth = 0; _OldHeight = 0; _OldImgName = ""; _OldImgPath = ""; _OldImgFormat = null; } public override string ToString() { return _OldImgPath; } public void ImageSave() { Bitmap tempBmp = new Bitmap(_OldImgPath); Bitmap bmp = new Bitmap(tempBmp, _NewWidth, _NewHeight); bmp.Save(_NewImgPath + @"\" + _NewImgName + "." + _NewImgFormat.ToString().ToLower(), _NewImgFormat); } public Bitmap ImageClone() { Bitmap bmp = new Bitmap(_OldImgPath); return bmp; } Bitmap IPicture.ReturnImage(string path, int width, int height) { return new Bitmap(new Bitmap(path), width, height); } } 

Main class; The starting point of the application. It definitely needs some work ...

 public partial class Form1 : Form { static bool hasThreadBeenStopped = false; static bool imageProcessingComplete = false; static bool imgConstrained = false; //Default text when user selects 'All' checkbox for new image name static string newNameDefault = "'Name' + #"; Utility.Validation.Validate valid = new Utility.Validation.Validate(); public Form1() { InitializeComponent(); //Populate Combo Box With Possible Image Formats... //Conditionally show Image Properties... ImgPropertiesEnabled(); //Set static progress bar properties... progressBar1.Minimum = 0; progressBar1.Step = 1; } private void Form1_Load(object sender, EventArgs e) { lblImgProcessed.Text = ""; lblFile.Text = ""; txtContentFolder.Text = ""; } //Delegate declarations. Used for multi-thread processing public delegate void PopulateTextboxDelegate(Label lbl, string text); public delegate void ThreadWorkDelegate(Label lbl, string text); public delegate void ImageDisplayDelegate(Image i); public delegate void ProgressBarDelegate(ProgressBar p, int step, int value); //Populate textbox fields with image processed, and image path being processed public void PopulateTextbox(Label lbl, string text) { lbl.Text = ""; lbl.Text = text; } public void ThreadWork(Label lbl, string text) { this.Invoke(new PopulateTextboxDelegate(PopulateTextbox), new object[] { lbl, text }); } //Display Currently Processed Image public void ImageDisplay(Image i) { pbMain.Image = null; pbMain.Image = i; } public void ThreadWorkImg(Image i) { this.Invoke(new ImageDisplayDelegate(ImageDisplay), new object[] {i}); } //Increment Progress Bar public void ProgressBarDisplay(ProgressBar pg, int max, int value) { //Dynamically set the Progress Bar properties pg.Maximum = max; pg.Value = value; } public void ThreadProgress(ProgressBar p, int max, int value) { this.Invoke(new ProgressBarDelegate(ProgressBarDisplay), new object[] { p, max, value }); } private void btnStart_Click(object sender, EventArgs e) { string IsValidResult = IsValid(); //If string is empty, Utility passed if (IsValidResult == "") { Thread t = new Thread(new ThreadStart(ProcessFiles)); t.Start(); } else { MessageBox.Show(IsValidResult); } } public void ProcessFiles() { int count = 0; ThreadWorkDelegate w = ThreadWork; ImageDisplayDelegate im = ThreadWorkImg; ProgressBarDelegate pb = ThreadProgress; try { foreach (MyPictures mp in lstHold.Items) { try { if (hasThreadBeenStopped == false) { //Disable certain controls during process. We will use the generic //MethodInvoker, which Represents a delegate that can execute any method //in managed code that is declared void and takes no parameters. //Using the MethodInvoker is good when simple delegates are needed. Ironically, //this way of multi-thread delegation was used because the traditional way as used //by the rest of the delegates in this method, was not working. btnApply.Invoke(new MethodInvoker(delegate { btnApply.Enabled = false; })); btnStart.Invoke(new MethodInvoker(delegate { btnStart.Enabled = false; })); //Call delegate to show current picture being processed im.BeginInvoke(mp.ImageClone(), null, null); mp.ImageSave(); //Increment Count; Image has been processed count++; //Invoke Img Proceessed Output w.BeginInvoke(lblImgProcessed, count.ToString() + " of " + lstHold.Items.Count.ToString() + " processed", null, null); //Invoke File Process Output w.BeginInvoke(lblFile, mp.NewImgPath, null, null); //Invoke Progressbar output. Delegate is passed The count of images, //which will be set as the progressbar max value. the 'count' variable is //passed to determine the current value. pb.BeginInvoke(progressBar1, lstHold.Items.Count, count, null, null); } else //Thread has been called to stop { MessageBox.Show("Image Processing Stopped: " + count + "of " + lstHold.Items.Count + " processed"); //Enable controls after process btnApply.Invoke(new MethodInvoker(delegate { btnApply.Enabled = true; })); btnStart.Invoke(new MethodInvoker(delegate { btnStart.Enabled = true; })); break; } } catch (Exception ex) { MessageBox.Show("Error while processing pictures"); break; } } } catch (Exception ex) { MessageBox.Show("Error while attempting to execute pictures: " + ex.ToString()); } finally { //Loop has ended: //In finally statement, re-enable disabled controls //Enable certain controls during process btnApply.Invoke(new MethodInvoker(delegate { btnApply.Enabled = true; })); btnStart.Invoke(new MethodInvoker(delegate { btnStart.Enabled = true; })); //Reset class variables hasThreadBeenStopped = false; imageProcessingComplete = false; } } private void btnContent_Click(object sender, EventArgs e) { string selection = null; string[] files = null; lstAll.Items.Clear(); contentBrowser.ShowDialog(); selection = contentBrowser.SelectedPath; txtContentFolder.Text = selection; if (selection != "" || selection != null) { try { files = System.IO.Directory.GetFiles(selection.Trim()); foreach (string file in files) { lstAll.Items.Add(file); } } catch (Exception ex) { // MessageBox.Show(ex.ToString()); } } } private void btnGo_Click(object sender, EventArgs e) { //Grab files from folder based on user input in the textbox. string selection = txtContentFolder.Text.Trim(); string[] files = null; lstAll.Items.Clear(); if (valid.IsNull(selection) == false || valid.IsEmpty(selection) == false) { try { files = System.IO.Directory.GetFiles(selection); foreach (string file in files) { lstAll.Items.Add(file); } } catch (Exception ex) { MessageBox.Show("Invalid Directory"); } } txtContentFolder.Text = selection; } private void btnDestination_Click(object sender, EventArgs e) { string selection = null; destinationBrowser.ShowDialog(); selection = destinationBrowser.SelectedPath; txtNewImgPath.Text = selection; } private void exitToolStripMenuItem_Click(object sender, EventArgs e) { this.Close(); } private void btnStop_Click(object sender, EventArgs e) { //Flag variable that the stop button has been called. This variable is checked //conditionally when looping over each picture. hasThreadBeenStopped = true; } public string IsValid() { StringBuilder sb = new StringBuilder(""); if (lstHold.Items.Count <= 0) { return "No items exist to process"; } //Validate that there is a value in each field for every object in lstHold. All the fields will be //validated. Note: If there is one invalid field, the rest do not need to be considered. foreach (MyPictures mp in lstHold.Items) { if (mp.NewImgName == "") { sb.Append(mp.OldImgPath + ", "); } else if (mp.NewImgPath == "") { sb.Append(mp.OldImgPath + ", "); } else if (mp.NewImgFormat == null) { sb.Append(mp.OldImgPath + ", "); } else if (mp.NewWidth == 0) { sb.Append(mp.OldImgPath + ", "); } else if (mp.NewHeight == 0) { sb.Append(mp.OldImgPath + ", "); } } //If the returned string is empty, the image is valid. The check for the listbox count //will return a string immediatly if false. Because of this, we know that the returning //string at this level will either be empty (validation passed) or filled with image paths //of images missing required values. If image is not valid, return this concatenated string of image paths //that are missing values, and insert a prefixed string literal to this list. if (sb.ToString() != "") { sb.Insert(0, "The following images are missing required values: "); return sb.ToString(); } else //String is empty and has passed validation { return sb.ToString(); } } private void btnMoveOne_Click(object sender, EventArgs e) { //Loop through All strings in the lstAll list box. Then use each picture path to convert //each picture into their own class foreach (string file in lstAll.SelectedItems) { //isImgExistFlag is a flag indicating wheter the image coming from lstAll already exists //in lstHold. By default, the variable is false. It is set to true if an image does exist //This variable must be re-created within the scope of the main foreach loop to ensure a proper //reset of the variable for each image comparison. bool isImgExistFlag = false; try { Image img; img = Image.FromFile(file); MyPictures mp = new MyPictures(img,file); //If lstHold contains no items, add the item with no validation check. if (lstHold.Items.Count == 0) { lstHold.Items.Add(mp); } else { //Run through each object in the lstHold to determine if the newly created object //already exists in list box lstHold. for (int i = 0; i < lstHold.Items.Count; i++) { MyPictures p = (MyPictures)lstHold.Items[i]; //Unique objects will be identified by their Original Image Path, because //this value will be unique if (p.OldImgPath == mp.OldImgPath) { isImgExistFlag = true; } } //If isImgExistFlag is false, the current Image object doesnt currently exist //in list box. Therefore, add it to the list. if (isImgExistFlag == false) { lstHold.Items.Add(mp); } } } catch (Exception ex) { MessageBox.Show(ex.ToString()); } } } private void btnMoveAll_Click(object sender, EventArgs e) { //This event has the same functionality as btnMoveOne_Click, except the main foreach loop //is based on all of lstAll items, rather than just the selected items. foreach (string file in lstAll.Items) { bool isImgExistFlag = false; try { Image img; img = Image.FromFile(file); MyPictures mp = new MyPictures(img, file); if (lstHold.Items.Count == 0) { lstHold.Items.Add(mp); } else { for (int i = 0; i < lstHold.Items.Count; i++) { MyPictures p = (MyPictures)lstHold.Items[i]; if (p.OldImgPath == mp.OldImgPath) { isImgExistFlag = true; } } if (isImgExistFlag == false) { lstHold.Items.Add(mp); } } } catch (Exception ex) { MessageBox.Show(ex.ToString()); } } } private void btnRemoveOne_Click(object sender, EventArgs e) { /* Create a seperate List to populate: This is necessary because if you explicitly remove an item from the listbox you will get the following error: "List that this enumerator is bound to has been modified. An enumerator can only be used if the list does not change." */ //This variable will keep track of the first index processed. int first_index = 0; int count = 0; List<MyPictures> TempMp = new List<MyPictures>(); if (lstHold.Items.Count >= 1) { try { foreach (MyPictures mp in lstHold.SelectedItems) { if (count == 0) { first_index = lstHold.SelectedIndex; } //Add objects to be removed TempMp.Add(mp); } foreach (MyPictures mp2 in TempMp) { lstHold.Items.Remove(mp2); } } catch (Exception ex) { //Hide Error: MessageBox.Show(ex.ToString()); } //Select new item in list if possible, as long as there is a item in the list if (lstHold.Items.Count >= 1) { //If the first_index variable = the amount of items in the list, the new selected index //should be the first index -1. This is because the variable first_index would be the //index of the now deleted item in the list. Therefore we must subtract the variable by 1 //before assigning it to the selected value. Otherwise, we'll be assigning a selected index that //no longer exists. //There is also a check to make sure there is more than one item in the list. Otherwise, we could //potentially assign a selected index of -1. if (first_index == lstHold.Items.Count && lstHold.Items.Count != 1) { lstHold.SelectedIndex = first_index - 1; } else if (lstHold.Items.Count == 1) { lstHold.SelectedIndex = 0; } else { lstHold.SelectedIndex = first_index; } } else { ClearTextBoxes(); } } } private void btnRemoveAll_Click(object sender, EventArgs e) { lstHold.Items.Clear(); ClearTextBoxes(); ImgPropertiesEnabled(); } private void lstHold_SelectedIndexChanged(object sender, EventArgs e) { //This prevents trying to access a negative index. This can happen when a item is removed. if (lstHold.SelectedIndex >= 0) { try { MyPictures mp = (MyPictures)lstHold.Items[lstHold.SelectedIndex]; txtOldName.Text = mp.OldImgName; txtOldImgPath.Text = mp.OldImgPath; txtOldImgFormat.Text = mp.OldImgFormat.ToString(); txtOldWidth.Text = mp.OldWidth.ToString(); txtOldHeight.Text = mp.OldHeight.ToString(); txtNewName.Text = mp.NewImgName; cbFormat.SelectedItem = mp.NewImgFormat; txtNewWidth.Text = mp.NewWidth.ToString(); txtNewHeight.Text = mp.NewHeight.ToString(); } catch (Exception ex) { MessageBox.Show(ex.ToString()); } } //Call function to determine which controls should be enabled/disabled ImgPropertiesEnabled(); } private void btnApply_Click(object sender, EventArgs e) { //Reset color. It could be grey depending on if user changed default name. txtNewName.ForeColor = Color.Black; if (lstHold.SelectedIndex == -1) { MessageBox.Show("Picture not selected. Select picture to apply properties to."); } else if (lstHold.SelectedIndex >= 0) { MyPictures mp = (MyPictures)lstHold.Items[lstHold.SelectedIndex]; //User wants to apply a generated name to all pictures within the list if (chkNewPicName.Checked == true) { int count = 0; foreach (MyPictures pic in lstHold.Items) { pic.NewImgName = txtNewName.Text + count.ToString(); ++count; } txtNewName.Text = mp.NewImgName; } //User wants to apply a custom name to this picture only else { mp.NewImgName = txtNewName.Text; } //User wants to apply this path to all pictures within the list if (chkNewPicPath.Checked == true) { foreach (MyPictures pic in lstHold.Items) { pic.NewImgPath = txtNewImgPath.Text; } txtNewImgPath.Text = mp.NewImgPath; } //User wants to apply this path to this picture only else { mp.NewImgPath = txtNewImgPath.Text; } //User wants to apply this image format to all pictures within the list if (chkNewPicFormat.Checked == true) { foreach (MyPictures pic in lstHold.Items) { pic.NewImgFormat = (ImageFormat)cbFormat.SelectedItem; } } //User wants to apply this image format to this picture only else { mp.NewImgFormat = (ImageFormat)cbFormat.SelectedItem; } //User wants to apply this size to all pictures if (chkNewSize.Checked == true) { foreach (MyPictures pic in lstHold.Items) { pic.NewWidth = Convert.ToInt32(txtNewWidth.Text); pic.NewHeight = Convert.ToInt32(txtNewHeight.Text); } txtNewWidth.Text = mp.NewWidth.ToString(); txtNewHeight.Text = mp.NewHeight.ToString(); } //User wants to apply this size to this picture only else { mp.NewWidth = Convert.ToInt32(txtNewWidth.Text); mp.NewHeight = Convert.ToInt32(txtNewHeight.Text); } mp.NewImgName = txtNewName.Text; mp.NewImgFormat = (ImageFormat)cbFormat.SelectedItem; mp.NewWidth = Convert.ToInt32(txtNewWidth.Text); mp.NewHeight = Convert.ToInt32(txtNewHeight.Text); } } private void checkBox1_CheckedChanged(object sender, EventArgs e) { if (chkSelectAll.Checked) { chkNewPicName.Checked = true; chkNewPicPath.Checked = true; chkNewPicFormat.Checked = true; chkNewSize.Checked = true; } else { chkNewPicName.Checked = false; chkNewPicPath.Checked = false; chkNewPicFormat.Checked = false; chkNewSize.Checked = false; } } private void previewToolStripMenuItem_Click(object sender, EventArgs e) { MessageBox.Show("hi there!"); } private void btnPreview_Click(object sender, EventArgs e) { try { if (lstHold.Items.Count <= 0) { MessageBox.Show("No pictures are available to preview"); } else if (lstHold.SelectedItem == null) { MessageBox.Show("No picture is selected to preview"); } else { MyPictures mp = (MyPictures)lstHold.SelectedItem; //Bitmap bmp = new Bitmap(mp.OldImgPath); Form2 frm = new Form2(mp); frm.Show(); } } catch (Exception ex) { MessageBox.Show("An Error has occured:\n " + ex.ToString()); } } public void ImgPropertiesEnabled() { //Enable Image properties when an image is selected if (lstHold.SelectedIndex >= 0) { gbCheckAll.Enabled = true; gbImgProperties.Enabled = true; } else { //Disable Image properties when an image is not selected gbCheckAll.Enabled = false; gbImgProperties.Enabled = false; } //Preview buttons enablement will depend on the same conditions btnPreview.Enabled = gbImgProperties.Enabled; } public void ClearTextBoxes() { txtNewImgPath.Text = ""; txtNewName.Text = ""; txtNewHeight.Text = Convert.ToString(0); txtNewWidth.Text = Convert.ToString(0); cbFormat.SelectedItem = null; chkSelectAll.Checked = false; } 

}

Answer 1

Here are some improvements for your code and design. These tips are not all OO related, but you should be aware that good design is not just OO design.

1. Avoid commenting on what is obvious.

 //Declare variables to hold values that have been determined private int _OldWidth; 

This comment is superfluous, because any programmers will understand that this is an announcement.

2. Avoid giving the wrong name. For example, the "MyPictures" class is not quite right because:

• It has only one image, while the name implies many images.
• It contains "My", which, in my opinion, is incorrect, because if I read your code, this is not my class. This is yours;)

3. Avoid string concatenation. Use string.Format or, for paths, Path.Combine

 bmp.Save(_NewImgPath + @"\" + _NewImgName + "." + _NewImgFormat.ToString().ToLower(), _NewImgFormat); 

4. Set the methods short. It is difficult to save all methods in 5 lines of code, but 30 lines (if my account is correct - without comments and empty lines) for ProcessFiles is a little too much.

5. Do not use design elements just because you want to use them. I see no reason to use the interface in your code. In your case, this just increases the complexity of your code. Moreover, you did not use it ( !!! ). You just implemented it and that’s it. Use interfaces when you have several types that have common functionality (those that are in the interface), and you want to treat all of them the same way, not knowing about the actual implementation.

 interface IImage { void DrawLine(Point startPoint, Point endPoint); } class MonochromeImage:IImage { void DrawLine(Point startPoint, Point endPoint) { //Draw a monochrome line on images with one channel } } class ColorImage:IImage { void DrawLine(Point startPoint, Point endPoint) { //Draw a red line on images with three channels } } ... void DrawLineOnImage() { List<IImage> images = new List<IImage>(); images.Add(new ColorImage()); images.Add(new MonochromeImage()); //I am not aware of what kind of images actually have //all it matters is to have a draw line method foreach(IImage image in images) { image.DrawLine(p1,p2) } } 

6. As already mentioned, try to separate the presentation (graphical user interface - GUI) from the logic. Do this in such a way that you can replace the GUI without changing the logic code.

7. Do separate responsibility functions. btnMoveOne_Click has more than one responsibility: it checks to see if the file exists and processes the elements in the user interface.

8.You image class is associated with the file system. What happens if I want to save images created in memory? What is the way then? Here you can improve the class design. Do it in such a way that it doesn’t matter if the files are on disk (HINT: in FileStream) or in memory (HINT: in MemoryStream) or anywhere else.

This is all for now. Hope this information helps you.

Answer 2

Having scanned the code, yes, this is eleborate ... maybe a little to a lot;)

One thing I noticed is your naming conventions. Although nothing has changed at run time, it simplifies API / code maintenance.

So, instead of having IPicture, I would make it something like "IResizableImage" (reading your specification, this is what it is. Not just an image, but resizing) Instead of 'ReturnImage ()', I would use something like 'scale ()'. 'ImageSave ()' in 'Save ()'

Your code will start reading (what is the added naming convention symposium)

  IResizableImage myImg = new ResizableImage (orignalBitmap);
 Image rescaledImg = myImg.Scale ("new path", 320,240);
 resccaledImg.Save ();

instead:

  IPicture myImg = new MyPictures ();
 Image rescaled = myImg.ReturnImage ("newpath", 320, 240);
 rescaledImg.ImageSave ();

So, usually classes are nouns, methods are verbs, and attributes are properties / fields. Try to minimize duplication or short duration. "ImageSave" is a method on your image. Isn't "Image.Save ()" clearer than "Image.ImageSave ()"?

Just some of my thoughts; There is no absolute or incorrect right in the coding guide. Think of being a different person when using the API and LETTER API. Jump out of the “I know what he is doing” window and imagine that the user has never seen this API before. Does it feel natural and easily accessible?

Hope this helps,

Answer 3

To achieve good design you need to apply TDD (Test Driven Design).

Soon you will find that testability requires dividing a project into layers, such as views and business logic.

Start covering your project with tests and you won’t believe how quickly you encounter design inconsistencies.

Things will just get up and shout: "In no case will you check me!" Worst anemia is code buried in WinForms. What you can do is make it "modest." http://codebetter.com/blogs/jeremy.miller/archive/2007/05/23/build-your-own-cab-part-2-the-humble-dialog-box.aspx

Regarding design patterns, you should look at architectural patterns, not OOP patterns. The keywords you will be looking for are MVC, MVP, MVVM.

Answer 4

Well, that’s what I will do. This is probably different than what many people will do, but I think it is a pretty good, flexible design.

 public abstract class AbstractConverter : IImageHandler { public AbstractConverter(IImageHandler nextHandler) { output = nextHandler; } public void HandleImage(Bitmap b) { var outBitmap = Convert(b); output.HandleImage(outBitmap); } protected abstract Bitmap Convert(Bitmap input); private IImageHandler output; } public interface IImageHandler { void HandleImage(Bitmap b); } 

Now the rest is your application:

• Create AbstractConverter implementations to handle the individual transforms you want.
• Creating something that can combine and combine converters.
• Getting the original bitmap and displaying the final result.