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

C# Winform Amazon Application

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

Problem

This is my first program I've written ever. Don't hold back on me. Any help and feedback is appreciated. This code wont work because it requires some keys, but I censored them out, but it does compile for me and works fine. I just want some feedback on dumb things I did and how to make it better if anyone has the time to help a beginner that is self-learning.

This application is using the Amazon MWS Order API to retrieve unshipped orders in our account and these orders are displayed in a datagridview. It then allows to a user to select among these orders and print the selection's order information to an Excel invoice template, and prints 3 copies of the invoice, then repeats for the next order that was selected. The last button on the form named confirm allows input from the Tracking Number column in the datagridview to confirm the amazon order with their system. numbers for the shipment into amazon and confirms the order.

program.cs

using System.Windows.Forms;

namespace MWSAutomate
{
    class Program
    {
        public static void Main()
        {

            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            Application.Run(new Form1());

        }
    }
}


Form1.cs

```
using System;
using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Linq.Expressions;
using System.Net.Mime;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Windows.Forms.VisualStyles;
using System.Xml;
using MarketplaceWebServiceOrders;
using MarketplaceWebServiceOrders.Model;
using System.Xml.Linq;
using MarketplaceWebService;
using MarketplaceWebService.Samples;
using Microsoft.Office.Interop.Excel;
using CheckBox = System.Windows.Forms.CheckBox;

namespace MWSAutomate
{
public partial class Form1 : Form
{
private CheckBox chk = new CheckBox();

Solution

for starters you got some clean code. it's neat and orderly which is good.

now for the bad:

1) you might want to move your const values off to configuration. they look like configuration values. see:
https://stackoverflow.com/questions/12731683/c-sharp-app-config-in-winform

2) your methods are really long, try this for starters but read up on method refactoring:
https://sourcemaking.com/refactoring/smells/long-method

3) Really, really, really consider the names of your methods. One of them is 'ExecuteSqlTransaction'. If the real work being done is saving orders to database then call it likewise, 'SaveOrdersToDatabase'

4) This would be a big step, but when I do winforms I prefer to use something called the MVP pattern. This separates the form from the logic and you have a lot of logic in your form. Try looking up info on MVP for winform. Pluralsight has a great course on this:
https://app.pluralsight.com/library/courses/windows-forms-best-practices/table-of-contents

(if you do not have a subscription to pluralsight then becoming a Dev Studio Essentials member will give you 6 months free:
https://www.visualstudio.com/en-us/products/visual-studio-dev-essentials-vs.aspx)

4) Excel com dll wrappers need to be disposed of correctly:
https://stackoverflow.com/questions/9962157/safely-disposing-excel-interop-objects-in-c

5) using hard coded paths to disk on your machine may not be good if you're sharing the app. Consider using shared locations:
https://stackoverflow.com/questions/867485/c-sharp-getting-the-path-of-appdata

6) You need to give your UI elements meaningful names 'button_1' and 'button_2' don't cut it. This will totally bite you in the behind later when you come back to it and are confused what button method belongs to what. Try doing this for ALL your controls. You can set the control name in the properties window (magic button F4)

Context

StackExchange Code Review Q#122538, answer score: 3

Revisions (0)

No revisions yet.