[Okta Webinar] Learn how to a build a cloud-first strategyRegister Now

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 418
  • Last Modified:

What's the problem of these codes or better way to write it

hi All,

here is the code review question i got it

1.      What is wrong with this statement?

       public static ulong SecondsInADay = 86399;

2.      What is wrong with the first line of this method (I’ve changed some of the names etc., so it’s not bad naming convention)?

       public static Response DoStuff(ADUser user)
        {
                string search = "(cn=" + user.name.Trim() + ")";

                                …
        }
               

3.      What would be a better way of doing this?

        public static string GetDomain()
        {
            return "DLA.tcs.net";
        }

Thanks in Advance
0
Sha1395
Asked:
Sha1395
2 Solutions
 
AndyAinscowCommented:
>>here is the code review question i got it

Is this homework?  (We can't answer for you, we could help you to correct things you say howver).
0
 
Sha1395Author Commented:
AndyAinscow, its not a home work anyway, just try to figure out the expert answers.
0
 
AndyAinscowCommented:
The first statement is just incorrect.  (Either the name is misleading or it is correct but the value given is wrong).  (Also have variables as public is not recommended - use get/set - and it is probably is meant to be a const).

The second and third use hard coded values (BUT that might not be a problem).


In general only use static when it is really required.  (If you don't understand what static does then look in the help files, it has a rather special effect.)
0
Industry Leaders: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
cubaman_24Commented:
Hello:
Your first code is not CLS compliant.
http://msdn.microsoft.com/en-us/library/bhc3fa7f.aspx

In the second case, you are risking an NullReferenceException, as user.name might be null, and the call to Tream() will fail.

I don't understand the las one.. But to get current domain, the right way is:
NetworkInformation.IPGlobalProperties.GetIPGlobalProperties().DomainName;

Best regards.
0
 
käµfm³d 👽Commented:
In general only use static when it is really required.
I have come to learn that there are varying schools of thought on the usage of static. One of those schools says to use static whenever there is no reason to maintain state (something classes excel at doing). In that case, the method would satisfy that requirement assuming the "..." doesn't maintain any sort of state.

What would be a better way of doing this?
I might do a readonly property:

e.g.

public string Domain
{
    get { return "DLA.tcs.net"; }
}

Open in new window

0
 
AndyAinscowCommented:
Don't forget if the name in the first code segment is correct then number of seconds in a day is 24x60x60 which is NOT 86399
0

Featured Post

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Tackle projects and never again get stuck behind a technical roadblock.
Join Now