C#•5mo ago

✅ how can i improve this code ?

c#

c#

message.txt

46 Replies

Joschi•5mo ago

$code

MODiX•5mo ago

To post C# code type the following: ```cs // code here ``` Get an example by typing $codegif in chat For longer snippets, use: https://paste.mod.gg/

Joschi•5mo ago

Also #code-review may be more applicable if you don't have a specific question.

Esa•5mo ago

Either channel works fine I think, there's generally a lot to tackle here. I did give you some advice earlier which is still applicable for this block of code.

steven preadlyOP•5mo ago

i know but most of them are formatiing issues , what i mean as i asked in the channel do this code has a performace issues nested do whils can be solved by spliting the code into small methods

Esa•5mo ago

It depends™. Performance can be measured in several ways, personally I consider readability a performance index, since that directly affects how long it'll take me to understand and modify a codebase - and therefore it affects developer output. Then there's stuff like cyclomatic complexity (how many different branches are there, such as if-statements, switches, do-while loops etc) as well as time complexity which you mentioned. If you do not agree that readability is a performance index, then you may honestly make the argument that the code you shared is fine as-is.

steven preadlyOP•5mo ago

can you refine this code for me if have time in a way that meke it more readable and do the same functionality i throught that it is like an alternative of the ? because as far as i know ? operator here say that this feild or prop may be null so it has the same function make null checkes ok @Fyren i am refining the code now

Angius•5mo ago

I'd at least extract some of those loops into their own methods, and I'd use the initializer for the list instead of calling .Add() a bajillion times Also, your Employee has no constructor but seems to require some of those properties. If so, I'd make them required at least

steven preadlyOP•5mo ago

dont forget that i am a biggener

Angius•5mo ago

None of what I mentioned is an advanced topic In fact, VS should suggest using the initializer for the list, and doing that should be as easy as using a quick fix

steven preadlyOP•5mo ago

ok i will fix that

Unknown User•5mo ago

Message Not Public

steven preadlyOP•5mo ago

is that what you mean

c#
        public Company() 
        {
            listEmployees = new List<Employee>()
            {
                new Employee { EmployeeId = 1, Name = "Mike", Gender = Gender.Male },
                new Employee { EmployeeId = 2, Name = "Pam", Gender = Gender.Female },
                new Employee { EmployeeId = 3, Name = "John", Gender = Gender.Male},
                new Employee { EmployeeId = 4, Name = "Maxine", Gender = Gender.Female },
                new Employee { EmployeeId = 5, Name = "Emiliy", Gender = Gender.Female },
                new Employee { EmployeeId = 6, Name = "Scott", Gender = Gender.Male },
                new Employee { EmployeeId = 7, Name = "Todd", Gender = Gender.Male },
                new Employee { EmployeeId = 8, Name = "Ben", Gender = Gender.Male }
            };
        }

c#
        public Company() 
        {
            listEmployees = new List<Employee>()
            {
                new Employee { EmployeeId = 1, Name = "Mike", Gender = Gender.Male },
                new Employee { EmployeeId = 2, Name = "Pam", Gender = Gender.Female },
                new Employee { EmployeeId = 3, Name = "John", Gender = Gender.Male},
                new Employee { EmployeeId = 4, Name = "Maxine", Gender = Gender.Female },
                new Employee { EmployeeId = 5, Name = "Emiliy", Gender = Gender.Female },
                new Employee { EmployeeId = 6, Name = "Scott", Gender = Gender.Male },
                new Employee { EmployeeId = 7, Name = "Todd", Gender = Gender.Male },
                new Employee { EmployeeId = 8, Name = "Ben", Gender = Gender.Male }
            };
        }

Angius•5mo ago

Yep Could probably even replace new Employee with just new()

Unknown User•5mo ago

Message Not Public

steven preadlyOP•5mo ago

what are the reson of creating a constractor while i have props already ?

Angius•5mo ago

Without a constructor and with all properties being optional, you can create a new Employee() It will not have a name, an ID, or a gender You can create a new Employee { Name = "Bob" } without an ID or gender Constructor and/or required properties prevent that

steven preadlyOP•5mo ago

is the required is an attribute

Angius•5mo ago

No, it's a keyword public required string Name { get; set; }

steven preadlyOP•5mo ago

is it diffrent than

c#
        [Required]
        public int EmployeeId { get; set; }

c#
        [Required]
        public int EmployeeId { get; set; }

Angius•5mo ago

Yes Attributes are not ran on compile time. This particular one is often used for runtime validation The required keyword will not allow the code to compile if the property is not set

steven preadlyOP•5mo ago

here it is

c#
    public class Employee
    {
        public required int  EmployeeId { get; set; }

        public required string? Name { get; set; }

        public required Gender Gender { get; set; }

        public int salary { get; set; }
    }

c#
    public class Employee
    {
        public required int  EmployeeId { get; set; }

        public required string? Name { get; set; }

        public required Gender Gender { get; set; }

        public int salary { get; set; }
    }

Esa•5mo ago

And "record" instead of class, and "init" instead of set 🙂

steven preadlyOP•5mo ago

i take to mush time for refactoring the code to make it readable is that normal for beginners

steven preadlyOP•5mo ago

i finally refactored the code above

refactored.txt

steven preadlyOP•5mo ago

I have refactored the code above

Angius•5mo ago

Looks fine, at a glance A nitpick, but methods and properties use PascalCase for naming, not camelCase

steven preadlyOP•5mo ago

Ok I will change that all of this code is for trying indexers 😆 But I spent a lot of time to refactor it is that normal for beginner

Angius•5mo ago

I'd say so Before you get the feel for how code should be laid out, you will be writing spaghetti and refactoring it

steven preadlyOP•5mo ago

Yes my code is readable now as I was talking in the same thread with @Faryen about code readability But is there are programmers that write refactored code directly without spaghetti

Angius•5mo ago

I don't think I've ever written code that I didn't refactor to some degree later Except some tiny one-off toy projects But you get better and better at writing code that needs less and less refactoring And code that is easier to refactor

steven preadlyOP•5mo ago

Ok but this is normal at the beginning

Angius•5mo ago

Yes

steven preadlyOP•5mo ago

Thank you very much

Esa•5mo ago

I'll post an example in a bit on github for you showcasing how I would refactor it

steven preadlyOP•5mo ago

Is the code. Still not readable for you I spent mush time in refactor it an seperate each part in a method

Esa•5mo ago

It's readable 🙂 You did a good job. This is just to showcase the next few tips and changes I would make.

steven preadlyOP•5mo ago

https://paste.mod.gg/oggyetwtgwxh/0

BlazeBin - oggyetwtgwxh

A tool for sharing your source code with the world!

Esa•5mo ago

Didn't finish it, will do it tomorrow.

steven preadlyOP•5mo ago

No problem thank you

Esa•5mo ago

Allright so I mocked up something here: https://github.com/EivindAntonsen/RefactoringExample/blob/refactoring-example/ConsoleApp1/Program.cs This branch is a refactored version of the code you pasted up above. There's probably a lot of different ways to do this, and many ways this can get over-complicated, but this is how I'd think about solving this. * Split classes into their own files. * Changed Company from being an instanced class to a static one. * Kept Employee as a class (like you had it initially, and not a record as I suggested a bit hastily). * Changed some of the do-while loops into while(true) loops with break conditions inside instead. * Removed the accessors in your Company class in favor of more explicit methods with accurate names, to more clearly express the intent of the methods. * Improved error messages for bad user input. Just ask if any of this stuff is unclear. And to anybody else watching, let me know if you disagree with where I've taken the code for any reason, I'm also interested in learning if anybody thinks I've made some poor choices. The main loop happens in line 5-25, everything below that is just methods

Angius•5mo ago

Little nitpick, if I may: GetEmployeeIdByUserInput could be shortened a bit with an early return. And converting to string is not needed, we have some nice char helpers

int GetEmployeeIdByUserInput()
{
    Console.WriteLine("Please enter the employee id.");

    while (true)
    {
        char input = Console.ReadKey().KeyChar;
        Console.WriteLine();

        if (char.IsNumber(input))
        {
            return char.GetNumericValue(input);
        }

        Console.Error.WriteLine("Invalid input (must be a numerical value), try again.");
    }
}

int GetEmployeeIdByUserInput()
{
    Console.WriteLine("Please enter the employee id.");

    while (true)
    {
        char input = Console.ReadKey().KeyChar;
        Console.WriteLine();

        if (char.IsNumber(input))
        {
            return char.GetNumericValue(input);
        }

        Console.Error.WriteLine("Invalid input (must be a numerical value), try again.");
    }
}

(also, as a side note, it will work only for 10 employees since it uses a char) So probably something like

int GetEmployeeIdByUserInput()
{
    Console.WriteLine("Please enter the employee id.");
    int id;
    while (!int.TryParse(Console.ReadLine(), out id))
    {
        Console.Error.WriteLine("Invalid input (must be a numerical value), try again.");
    }
    return id;
}

int GetEmployeeIdByUserInput()
{
    Console.WriteLine("Please enter the employee id.");
    int id;
    while (!int.TryParse(Console.ReadLine(), out id))
    {
        Console.Error.WriteLine("Invalid input (must be a numerical value), try again.");
    }
    return id;
}

would be better

Unknown User•5mo ago

Message Not Public

Esa•5mo ago

@steven preadly make sure you check out this before it gets closed. 🙂 also ty @Angius , it was made with the current constraints in mind, but that was a good point. can fix that.

steven preadlyOP•5mo ago

hello thank you for this great refactor that you have made to the code , but i want you to notice that i made this code from the beginning to try indexers syntax , I am now studding it ,that's why i used indexers in my code , of course methods are less complicated than indexers and there are another ways that i can do the same functionality other than using indexers thank you very mush for helping me to refactor the code what dose this means

Gaming

Programming

✅ how can i improve this code ?