Click here to Skip to main content
15,867,568 members
Please Sign up or sign in to vote.
4.50/5 (2 votes)
See more:
Hi,

I started programming in C# this week. Always programmed in PHP but I am eager to learn another language. So hopefully you guys, the senior C# can review my code. I hope to learn new things from it.

Thanks in advance.

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;

namespace WebService {
    public partial class IncidentCreator : Form {

        public IncidentCreator() {
            InitializeComponent();

            VisibleFalse();
        }

        private void VisibleFalse() {

            //TextLabels
            labelTextDomain.Visible = false;
            labelTextUserID.Visible = false;
            labelTextShortDesc.Visible = false;
            labelTextDescribe.Visible = false;

            //ComboBox
            comboBoxDomain.Visible = false;

            //TextBox
            textBoxShortDesc.Visible = false;
            textBoxUserID.Visible = false;
            textBoxDescribe.Visible = false;

            //Button
            buttonCreateIncident.Visible = false;

        }

        private void VisibleTrue() {
            labelTextDomain.Visible = true;
            labelTextUserID.Visible = true;
            labelTextShortDesc.Visible = true;
            labelTextDescribe.Visible = true;

            comboBoxDomain.Visible = true;

            textBoxShortDesc.Visible = true;
            textBoxUserID.Visible = true;
            textBoxDescribe.Visible = true;

            buttonCreateIncident.Visible = true;
        }

        private void comboBoxIdentUser_SelectedIndexChanged(object sender, EventArgs e) {

            if (comboBoxIdentUser.SelectedItem.Equals("User X")) {

                Size = new Size(334, 376);

                VisibleTrue();

                var a = System.Security.Principal.WindowsIdentity.GetCurrent().Name.ToString();
                textBoxUserID.Text = a;

            } else if (comboBoxIdentUser.SelectedItem.Equals("User Y")) {

                VisibleFalse();
                Size = new Size(334, 110);

            }
        }

        private void buttonCreateIncident_Click(object sender, EventArgs e) {

            buttonCreateIncident.Enabled = false;
            comboBoxIdentUser.Enabled = false;
            comboBoxDomain.Enabled = false;
            textBoxShortDesc.Enabled = false;
            textBoxDescribe.Enabled = false;

            //Reference Code
            WebReference.Now_incident soapClient = new WebService.WebReference.Now_incident();
            System.Net.ICredentials cred = new System.Net.NetworkCredential("admin", "xxx");
            soapClient.Credentials = cred;

            WebReference.insert insert = new WebReference.insert();
            WebReference.insertResponse response = new WebReference.insertResponse();

            var a = System.Security.Principal.WindowsIdentity.GetCurrent().Name.ToString();
            var x = a;

            var b = x.Replace("Domain" + (char)92, "");

            insert.caller_id = b;
            insert.short_description = textBoxShortDesc.Text.ToString();
            insert.comments = textBoxDescribe.Text.ToString();

            try {

                response = soapClient.insert(insert);
                MessageBox.Show("Your Incident has been created: " + response.number + "\n");

            } catch (Exception error) {

                this.textBoxDescribe.Text = error.Message;

            }
        }
    }
}
Posted
Updated 28-Mar-11 21:51pm
v4
Comments
avigodse 29-Mar-11 5:46am    
Considering the code review and development standards at beginning is very good approach.
Keep it up.
Sergey Alexandrovich Kryukov 29-Mar-11 13:14pm    
Agree, see my vote of 5 and my review.
--SA
Sergey Alexandrovich Kryukov 29-Mar-11 12:36pm    
For a beginner, the code is not too bad (actually much better the 95% of other "professional" Inquirers in this site). The desire to get code review is also well motivated. Overall, I voted 5 for the Question.
--SA

1) Comments! Add them. Use the XML comment facility, it gets your methods and fields added to Intellisense.
2) Don't use "Magic Numbers" - (char)92 ? Why? 334, 376, 110? What? Use a constant instead, and give it a sensible name so I don't have to go hunting for what the heck it does!
3)
var a = System.Security.Principal.WindowsIdentity.GetCurrent().Name.ToString();
var x = a;
var b = x.Replace("Domain" + (char)92, "");
Why create "x" at all? You never use it again...
4) Personally, I hate "var" being used for defined, standard types: ToString always returns a string, so it it clearer to use
string a = System.Security.Principal.WindowsIdentity.GetCurrent().Name.ToString();
string b = a.Replace("Domain" + (char)92, "");

5) Naming: Why call it "a", or "b": "name" and "localName" are much more obvious to read.

There is probably more, but that'll do for a start! :laugh:
 
Share this answer
 
Comments
Member 7762422 29-Mar-11 4:51am    
Thanks, all comments are usefull in a learning curve!
If you are looking for the Code Review Tools, then there are many in market, but i have came across to some which are provided by Microsoft itself. And as per reliability and audit standards these are the closest.
If you are looking for after compilation review, this is the first option FxCop
And in the cased where review has to be done by developer him/her self or by tech.lead at Development Stage, here is the other option StyleCop
 
Share this answer
 
Comments
Member 7762422 29-Mar-11 6:12am    
Thanks, really a good advice to review my code :-) ...
Dalek Dave 29-Mar-11 13:02pm    
Good Call
I try to avoid code in Event Handlers (buttonCreateIncident_Click). I write the code as a function, and call it from the event handler (if i need it on another place ...)
 
Share this answer
 
Comments
Member 7762422 29-Mar-11 4:51am    
Thanks, all comments are usefull in a learning curve!
Dalek Dave 29-Mar-11 13:02pm    
Good Advice
The names like buttonCreateIncident_Click look ugly and violate Microsoft naming conventions. Yes, I know it is generated by Microsoft, so what? They are ugly and violate them. All auto-generated names imply you rename them semantically. In this way, you will indicate they were attended. See FxCop — it won't allow such names.

Better yet, never allow auto-generate events. Write them manually: it's faster (overall) and provides supportable code. Do this:

C#
MyButton.Click += (sender, eventArgs) => {
    //do something
    //if you call some method from here
    //you don't have to pass unused parameters sender or eventArgs,
    //you can pass whatever you want
    //this is lambda form, you don't event need to declare parameter types -- they are
    //inferred from event type
};


If you have to use C# v.2 you cannot use lambda, so you have to use this:

C#
MyButton.Click += delegate(object sender, System.EventArgs eventArgs) {
    //do something
;


Anyway, this is much better than your auto-generated code.

I also would advice to split all your code in a separate file. The form declaration is partial, so you can add another part in a separate file and isolate your code from auto-generated one.

Look at your "Domain", "(char)92", and especially "". You use so called immediate constant 92, that is, hard-coded. This is bad. Never do it in code. Such stuff belongs in resources (data, such as configuration files in some other cases). To simplify that, you can put all your constants in a separate static class, where you can explicitly declare them as constant. Don't use immediate except some trivial cases such as null, 0 and 1 (but not "", use string.Empty).

You should always clean-up you using clauses. Visual Studio has "Organize using" -> "Remove unused using", use it. It can be not enough. Go to your project's References and clean the first. The template usually auto-generate a lot of unused staff. Better add references when it is really needed, in order to support clean code.

—SA
 
Share this answer
 
v2
Comments
Member 7762422 29-Mar-11 12:28pm    
Thanks SA, usefull comments in your reply!!!
Sergey Alexandrovich Kryukov 29-Mar-11 12:33pm    
You're welcome.
Thanks for accepting this Answer.
It would be even better if you follow my advices -- they are tested by years of development.

Good luck, call again,
--SA

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900