So since Monday I've been working on my C# skills
and testing my self i decided to write a banking app simulation in c#

here is the link to the gist:
https://gist.github.com/TaqsBlaze/...

and link to the executable:
https://github.com/TaqsBlaze/...

to all the experts out there whats your advice/critics etc

There are no free code reviews on devRant 🤣.

Hahahaha...... well I am just looking for advice from professionals out there
I wrote it just for fun taking a brake from my main python project

It reminds me of my first days on C++.
I'm sorry I can't really help you on the code, I feel like it's a bit messy and you should maybe split your code into different files (like one for each class)

The only big thing I can see is that you're using recursion for withdraw and changepassword, when you could just use a do-while loop (save a bit of memory since it avoids using the same function over and over.

You might also want to consider using only the libraries you actually need.

I haven't read it thoroughly, but I noticed some unusual names of classes and methods which perhaps suggests a non conventional program structure over all.

The classes CreateAccount and Run, for example, sugget that they don't represent blueprints for objects but rather actions. Same but opposite can be said about the methods UserAccount and menu, which due to their naming suggest that they represent objects.

To be fair, I've never written C#, but these concepts are tied to object orientation and not C# specifically. Someone, correct me if there are nuances to this.

@ZioCain
Thanks for that advice
I am kind of embarrassed though
I know my code doesn't look neat but hey I've only taken c# seriously since this Monday

I will improve

@Taqsblaz3 don't worry! Everyone gotta start somewhere :D

1. consider using spaces at 4 wide at most, makes it easier to read in PRs. Most of the vs tools have standard conventions defined for use in autoformat, as does rider
2. Conventionally, the entry class is Program.cs
3. Have a look at NLog or Serilog to simplify your log writing strategies
4. Break up the application into separate files per class to make it more consumable
5. Consider this package to make your cmdline handling more consistent and more easily support actions and arguments:
https://github.com/natemcmaster/...
6. Consider making line 25 readonly, and make it
7. Make use of the singleton pattern for resources that never mutate (contexts, constants, etc)
8. Prefer IEnumerable<T>, IDictionary<TKey, TValue>, ISet<T> implementations to arrays
9. Prefer linq expressions to for loops. This is a time space tradeoff that will reduce your memory usage when coupled with properly async enumerables
10. Consider enabling C# 8.0 and nullable ref types
...

11. with 10, prefer switch expressions, pattern matching to if/else blocks. This ensures your code is finite and eliminates externalized tracking assignments. https://docs.microsoft.com/en-us/...
12. Some of the methods do more than 1 thing. Creating classes that encapsulate their domain concern and internally defining operations that act on the data context will allow you to more naturally break those operations up into smaller pieces that are DRY
13. Try catch blocks should always have a finally
14. also with 10, use switch expressions, linq where clauses, nullables, null conditionals and null coalescence to avoid manual null checks
15. declare most variables types as nullable to ensure that the compiler complains about null conflicts
16. make use of the null conditional assignment operator and null conditional for default values
17. Use the null forgiveness operator with switch expressions to allow for safe deref of potential null values
18. Use the trampoline pattern for recursive scenarios (C# does not support tail calls)

Some other things, but that's enough for a start. Applying those principles and tools, the final version of this code could be somewhere in the range of 40 lines of code doing actual work.

@SortOfTested Do you have a resource where you pulled those practices out of? Or are they a series of practices you've come to use over the years?

@vomitmachine
Been programming .Net since alpha. Top of my head sort of thing unfortunately.

@SortOfTested Nuts, some of those are language specific but some of them are general, keep your code clean suggestions. They're really good suggestions.

Mind if I ask a few questions why?

Your suggestion for the null variable/nullable type reliance comes up a few times. In some cases it makes sense to me, in other cases it doesn't. How has nullable variables made your programming easier?

Consider me ignorant on what you can do with a finally block in a try catch, but why would that need to be in every try catch. What would you use it to do? I can't think of much of a big driver unless you're using a try/catch to initialize some data that may error out like a data base connection or something.

@vomitmachine
I'll unpack this a little at a time:

- Null all the things
C# 8 added broad support for deep nullable and null-relational checks.

To my mind, I have two responsibilities when writing code and dealing with nulls:
- ensuring that my code appropriately handles nulls in all cases (even some I can't necessarily predict)
- ensuring that my code communicates to consumers that a null condition can exist and they need to handle it.

The 7.0 and 8.0 updates made this significantly more elegant, and in my opinion easier to deal with than condition checks all over the place. Gone are the days of x.HasValue ? ..., now you can just var tempVar ?: someValue?.that?.may?.be?.null ?? defaultValue.

Especially in distributed systems, this will routinely be an issue if data contracts, sanitation, validation or upstream null-handling changes.
...

@vomitmachine

It also allows me to specify in projects that unhandled nulls are compile time errors. So now the compiler assists me at screaming at users that they've left runtime errors open to change.

C# 6.0 also introduced null invocation, which presents useful options for conditional invocation:

if (doSomething?(args) ?? false) <- doesn't compile at all

if (doSomething?.Invoke(args) ?? false) <- compiles and is happy

This is super useful in situations where a property on a framework may be null if a condition isn't met. You'll see this commonly used in WPF plumbing.
...

@vomitmachine

-- Null all the things with switch expressions

This one is easy. I have a strong preference for low stack pressure, memory reuse and finite isolation of things. I also love tuples, and functional programming.

Assume you have a response that has a header that can be null and you need to generate an IActionResult based on it:

if(response.Status == HttpStatusCodes.Success && response.TryGetHeader("custom_header"), out string customHeader) && customHeader == "somevalue")
return Ok(200);
else
return new StatusCodeResult(500, "The service was unable to process the request in an acceptable fashion")
...

@vomitmachine

That's fugly. You can encapsulate it all like this instead, and also include easy to follow additional functionality for overrides or custom string types:
public IActionResult SomeFnResponseDiscriminator((int StatusCode, string? header, string? comp) args) => args switch
{
(200, "somevalue", null) => Ok(204),
(200, null, null) => new BadRequestResult(),
(200, _, _) => args switch
{
_ when args.StatusCode == 200 && args.header == args.comp => Ok(204),
_ => new StatusCodeResult(500)
},
_ => new StatusCodeResult(500)
};
public async Task<IActionResult> SomeFn
{
...
return SomeFnResponseDiscriminator((
(int)response.StatusCode,
response.Headers.TryGetValue("custom_header", out string result) ? result : null),
null

@vomitmachine

-- always include finally

This is a specific team of programming theory that says if you don't have a finalization, cleanup, remediation present, you're likely just swallowing an exception and continuing. Theory is, if you don't have anything that always needs to run, you probably didn't need the catch to begin with. Theory:

- if you have numerous try catches, your method probably does too much
- if an exception is non-terminal, you have a leaky abstraction
- if an exception can be logged in a method, it can also be logged by a global handler in your application's loop

composition:

{
try{ ... work to be done assembling a context... }
catch(...){ ... do something meaningful with non-terminal exceptions that also somehow allows this to continue }
finally{ ... result handling from context building operation ... }
}
));
}

@vomitmachine
Aaaaand that's before we start discussing records classes and all the C# 9.0 stuffs.

@SortOfTested That was definitely a lot of information but pretty helpful.

So a summary would be that nullables are heavily used in interfacing with data contexts and then those shorthand operators are used in conjunction with that to make your code easier to write/read/maintain.

And the try/catch/finally philosophy is basically don't use it unless try/catches unless you're dealing with code that could make your program actually fail. Or you can handle the problematic data context assignments if something bad happens like a population from active directory with a finally block. But don't sprinkle try/catch everywhere. I'm ironically running into that issue right now. Trying to unravel a bunch of hidden try/catches that silence errors instead of reporting them.

Thanks for writing that up, it looks like it took you some time but it's really helpful.

@vomitmachine
No prob. At this point it all just rolls off the brains onto the keyboard, I train my juniors all the time so it's mostly canned.

@SortOfTested I don't even think that advice only applies to juniors. It applies to everyone. There's a lot of people out there who don't conform to any sort of practices when they're coding and the ones you've suggested are a good starting point. I bet if you come up with a little manual or something it'd come in handy for more than a few people. Or at the very least some practices may be drawn out of it.