-3
\$\begingroup\$

I am new to coding, and I am doing my best to write efficient and robust code. I am also trying to incorporate design patterns into my code. Could you please take a look at my repository and let me know if I am on the right track?

 public IPaymentGateway GetPaymentGateway(string paymentMethod)
    {
        if (PaymentGateways.TryGetValue(paymentMethod, out IPaymentGateway paymentGateway))
        {
            return paymentGateway;
        }
        else
        {
            throw new ArgumentException("Invalid payment method");
        }
    }

GitHub

\$\endgroup\$
0

1 Answer 1

0
\$\begingroup\$

There's not much Review Context in this submission. The very brief amount of code looks correct, yes.

You showed us just a library routine, and did not show us the calling code, which might be part of app-level code or part of an automated test suite.

informative diagnostics

            throw new ArgumentException("Invalid payment method");

This is perfectly nice as-is.

A maintenance engineer investigating some issue that cropped up in Production will want to know the value of paymentMethod. You could be kind to that person, and include it as part of this exception's message.

model != modal

Those two English words mean different things.

https://github.com/thomascharles9212/PaymentIntegration/blob/main/PaymentIntegrations/Modals/Payment.cs

namespace PaymentIntegrations.Modals
{
    public class Payment
    {
        public string Id { get; set; }
        ...
        public DateTime CreatedAt { get; set; }

Maybe this is "normal" boilerplate in this community? IDK. I tend to think of an Id as immutable once it's created. And offering a setter for CreatedAt doesn't seem sensible, since it's a historic fact which won't change over the lifetime of the record.

unimplemented

You invited us to take a look around, without any direction on what to examine. For example, there's no test suite which someone might single-step through to visit "important" methods which deliver business value. I do not yet see the relationship between payments and WeatherForecast.

https://github.com/thomascharles9212/PaymentIntegration/blob/main/PaymentIntegrations/Services/StripePaymentGateway.cs

            throw new NotImplementedException();
            ...
            throw new NotImplementedException();
            ...
            throw new NotImplementedException();
            ...
            throw new NotImplementedException();
            ...
            throw new NotImplementedException();
            ...
            throw new NotImplementedException();

Not sure that there's much to say about this code base.

\$\endgroup\$
2
  • \$\begingroup\$ I appreciate the in-depth review. Do I need to implement a layer structure for this project, such as separating the BLL and DAL? I'm asking now so that if I need to make any changes, I can do so early on. If I'm heading in the right direction, I'd like to keep moving forward. \$\endgroup\$ Apr 2 at 19:11
  • \$\begingroup\$ During an end-of-sprint code review we answer a pair of questions about code in the feature branch: (1) is it correct? (2) is it maintainable? If "yes" and "yes", then the team can accept the new software artifact as a shared artifact, which anyone on the team could maintain. What I didn't see in your submission was a use case being addressed, or some concrete business value being added to the codebase. We worry about Maintainability because we know in future some bug fix or new feature will have to be added. If I can't thread my way through a current use case, I can't predict future ones. \$\endgroup\$
    – J_H
    Apr 2 at 19:30

Not the answer you're looking for? Browse other questions tagged or ask your own question.