1
\$\begingroup\$

I wrote a simple currency converter in Python. Right now, my currency converter converts using the last available rate or converts using a specific date.

Looking for:

  • I'd appreciate any tips about how I can improve my little script.
  • What new features would be nice to add to my project?

Many thanks!

PS. I'm very sorry for my bad English, I'm not a native speaker.

from currency_converter import CurrencyConverter
from datetime import date
from colorama import Fore
import datetime

class CC():
    def __init__(self):
        self.features = ["1. Convert using the last available rate.", "2. 
Convert using specific dates."]
        print("")
        print(Fore.GREEN + "-" * 76)
        print("")
        for x in self.features:
            print(x)
        print("")
        self.select = input("Select a feature: ")
        print("")
        print(Fore.GREEN + "-" * 76)
        print("")
        self.amount = input("Amount: ")
        self.c1 = input("Currency1: ")
        self.c2 = input("Currency2: ")
        self.c = CurrencyConverter()
        if self.select == "2":
            self.year = int(input("Year: "))
            self.month = int(input("Month: "))
            self.day = int(input("Day: "))

    def input_process(self):
        if self.select == "1":
            self.d = round(self.c.convert(self.amount, self.c1, self.c2), 2)
            self.x = datetime.datetime.now()
            print(f"{self.amount} {self.c1} is {self.d} {self.c2} on {self.x}")
        elif self.select == "2":
            self.d = round(self.c.convert(self.amount, self.c1, self.c2, date=date(self.year, self.month, self.day)))
            print(f"{self.amount} {self.c1} was {self.d} {self.c2} in {self.year}-{self.month}-{self.day}")



if __name__ == '__main__':
    currencies = ["USD", "SEK", "GBP", "EUR"]
    print(Fore.GREEN + f"This is a Currency converter, the main currencies is {', '.join(currencies)}")
    print("")
    print(Fore.GREEN + "-" * 76)
    print("")
    while True:
        test = CC()
        test.input_process()
        print("")
        print(Fore.GREEN + "-" * 76)
        print("")
        if input("Finished. Do another? (Y/N)\n").lower() != 'y':
            break
\$\endgroup\$
  • 2
    \$\begingroup\$ Fix indentation, code is defective and this might make it considered as off topic on this website (if the code works in the first place) if it doesn't then you should move it to stack overflow. \$\endgroup\$ – user203258 Sep 15 at 19:43
  • 2
    \$\begingroup\$ from currency_converter import CurrencyConverter where is this module? \$\endgroup\$ – user203258 Sep 15 at 20:16
  • 1
    \$\begingroup\$ @EmadBoctor pypi.org/project/CurrencyConverter \$\endgroup\$ – Arian Chimenson Sep 15 at 20:34
  • 5
    \$\begingroup\$ It would be worth including that information in the question body, as comments have a habit of disappearing. It would also be nice if you could include a description of what the code does (e.g. how does it take input; what are the outputs). 'Currency Converter' is, I think, too vague. \$\endgroup\$ – VisualMelon Sep 16 at 9:12
4
\$\begingroup\$

First, there's very little point in using a class like you are here. You're essentially just using the constructor as a function to ask for input. You're also needlessly making some variables attributes of the object, like self.features, self.x, and self.d. The only use of features is to be iterated over within the constructor, and the only use of the latter two are inside of input_process. Even if this were an appropriate use of a class, that data doesn't need to be retained in the object itself. That just wastes memory.

To remedy this, I'd take everything in __init__, and move it into a regular function. To store the data, I'd use a NamedTuple returned from the function.


Your prints are also needlessly verbose. You're using print("") as a way to print newlines. Newlines can simply be added using "\n" though.

print("")
print(Fore.GREEN + "-" * 76)
print("")

Can simply be:

print("\n" + (Fore.GREEN + "-" * 76) + "\n")

And since you use (Fore.GREEN + "-" * 76) multiple times, I'd save that into a string at the top of the file:

SEPARATOR = "\n" + (Fore.GREEN + "-" * 76) + "\n"

Your loop over features is also unnecessary. It can be done simply using join:

print("\n".join(features))

Your names are also poor. c1, c2, c, x and d are very vague and give little information about what they hold. I would make those names much longer.



After making the mentioned changes, and I ended up with:

from currency_converter import CurrencyConverter
from datetime import date
from colorama import Fore
import datetime
from typing import NamedTuple, Optional, Tuple

# These are constants. They should be defined externally, and have capitalized names
FEATURES = ["1. Convert using the last available rate.", "2. Convert using specific dates."]
SEPARATOR = "\n" + (Fore.GREEN + "-" * 76) + "\n"

class ConversionInformation(NamedTuple):  # A simple class to hold information
    feature_type: str
    amount: str
    source_currency: str
    target_currency: str
    conversion_date: Optional[Tuple]

def ask_for_input() -> ConversionInformation:
    print(SEPARATOR)
    print("\n".join(FEATURES))

    feature_type = input("Select a feature: ")

    print(SEPARATOR)

    amount = input("Amount: ")
    source_cur = input("Currency1: ")
    target_cur = input("Currency2: ")

    conversion_date = None

    if feature_type == "2":
        conversion_date = (int(input("Year: ")),
                           int(input("Month: ")),
                           int(input("Day: ")))

    return ConversionInformation(feature_type, amount, source_cur, target_cur, conversion_date)

def process_input(info: ConversionInformation):
    conv = CurrencyConverter()  # This wasn't needed when asking for input

    if info.feature_type == "1":
        # Note the more descriptive names
        result = round(conv.convert(info.amount, info.source_currency, info.target_currency), 2)
        cur_date = datetime.datetime.now()

        print(f"{info.amount} {info.source_currency} is {result} {info.target_currency} on {cur_date}")

    elif info.feature_type == "2":
        result = round(conv.convert(info.amount, info.source_currency, info.target_currency,
                                    date=date(*info.conversion_date)))

        print(f"{info.amount} {info.source_currency} was {result} {info.target_currency} in" +
              "-".join(str(d) for d in info.conversion_date))

    else:
        print("Illegal Feature Type:", info.feature_type)  # Tell the user that they entered an illegal type


# Also a constant
CURRENCIES = ["USD", "SEK", "GBP", "EUR"]


if __name__ == '__main__':
    print(Fore.GREEN + f"This is a Currency converter, the main currencies are {', '.join(CURRENCIES)}")
    print(SEPARATOR)

    while True:
        info = ask_for_input()
        process_input(info)

        print(SEPARATOR)

        if input("Finished. Do another? (Y/N)\n").lower() != 'y':
            break

It's far more verbose, but I believe it's much cleaner and easier to understand.

My main issue with what I've done here is package all the data into ConversionInformation just so it can be passed into process_input. You may find that it ends up being cleaner to make this all one large function.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback! I appreciate your help, and I'm sorry If I'm an annoying asshole. \$\endgroup\$ – Arian Chimenson Sep 16 at 12:33
  • 1
    \$\begingroup\$ @ArianChimenson Np. And not sure what you're referring to. If you're talking about how your code wasn't indented properly, it was fixed by the time I saw it. No worries. \$\endgroup\$ – Carcigenicate Sep 16 at 13:17

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