2
\$\begingroup\$

I have a console application, which uses Ndesk.Options to parse console arguments, request weather data and print them. I am looking primarily for code style suggestions.

This is the code, which parses the console arguments:

var argSet = new OptionSet() {
    { "l|location=", "Can be a city or a zip-code. Add a countrycode or a countryname to specify location. Use ISO 3166 codes. Example: Berlin,Germany", (loc) =>
    {
        var locAndCode = loc.Split(',');
        if(locAndCode[0].All(ch => char.IsDigit(ch)))
        {
            zipCode = int.Parse(locAndCode[0]);
            requestMethod = RequestMethod.ZipCode;
        }
        else
        {
            city = locAndCode[0];
            requestMethod = RequestMethod.City;
        }
        if(locAndCode.Length > 1)
        {
            if(CountryCodeHelper.IsValidCode(countryCode)) countryCode = locAndCode[1];
            else countryCode = CountryCodeHelper.ConvertToCode(locAndCode[1]);
        }
    }
    },
    {"d|days=", "Days of weather to receive. 0 for current weather, 4 days forecast is avaible.", (int v) => 
    {
        days = v <= 4 ? v : 4; 
    }
    },
    { "h|help", (help) => showHelp = help != null  },
    {
    "u|units=", "Determites the units. Avaible values are Default, Metric and Imperial. For more information see the openweathermap documentation" ,(string v) =>
    {
        Enum.TryParse(v, out units);
    }
    }
};
try
{
    argSet.Parse(args);
    if(requestMethod == RequestMethod.None && !showHelp) throw new OptionException("No location was given!", "location");
}
catch(OptionException opEx)
{
    PrintError(opEx.Message);
    Console.WriteLine("Try --help for more information.");
    return;
}
if(showHelp)
{
    ShowHelp(argSet);
    return;
}

This methods are used to print help and errors:

static void ShowHelp(OptionSet argSet)
{
    Console.WriteLine("Usage: openWeather [OPTIONS]");
    Console.WriteLine("Prints the weather for the given location.");
    argSet.WriteOptionDescriptions(Console.Out);
}
static void PrintError(string message)
{
    Console.ForegroundColor = ConsoleColor.Red;
    Console.WriteLine("[ERR]");
    Console.ForegroundColor = ConsoleColor.White;
    Console.WriteLine(message);
}

After parsing and request of the weather data it's printed:

//This is placed in main//
 PrintLocation(currentWeather);
 ChangeColor(ConsoleColor.Magenta);
 Console.WriteLine("== Current weather ==");
 PrintWeather(currentWeather);
 if(days > 0)
    {
        DateTime printDate = DateTime.Now.Date;
        for(int i = 0; i < days;i++)
        {
            printDate = printDate.AddDays(1);
            ChangeColor(ConsoleColor.Magenta);
            Console.WriteLine("== Day {0}/12:00am ==",printDate.ToShortDateString());
            PrintWeather(forecast.TimePoints.Single((weatherTimePoint) => weatherTimePoint.TimePoint.Date == printDate && weatherTimePoint.TimePoint.Hour == 12));
        }
    }


static void PrintWeather(IWeatherSet value)
{
    ChangeColor(ConsoleColor.DarkGreen);
    Console.Write(value.Weather[0].Condition);
    ChangeColor(ConsoleColor.White);
    Console.WriteLine("({0})", value.Weather[0].Description);

    Console.Write("Temperature:");
    ChangeColor(ConsoleColor.Red);
    Console.WriteLine("{0}{1}", value.ClimaticData.Temperature, UnitsHelper.GetUnit(value.Units, WeatherField.Temperature));

    ChangeColor(ConsoleColor.White);
    Console.WriteLine("Humidity:{0}%",value.ClimaticData.Humidity );
    Console.WriteLine("Pressure:{0}hPa",value.ClimaticData.Pressure);
    Console.WriteLine("Wind speed:{0} {1}",value.Wind.Speed,UnitsHelper.GetUnit(value.Units,WeatherField.WindSpeed));
    Console.WriteLine("Wind degree:{0}°",value.Wind.Direction);
}
static void PrintLocation(ILocation value)
{
    ChangeColor(ConsoleColor.White);
    Console.Write("Weather for: ");
    ChangeColor(ConsoleColor.DarkCyan);
    Console.Write(value.City);
    ChangeColor(ConsoleColor.White);
    Console.WriteLine(", {0}:", value.Country);
}

The complete code can be looked up at the GitHub page.

\$\endgroup\$
2
\$\begingroup\$
  • The initialisation of argSet is way too long to be plonked right in the middle of Main(). It distracts from what that method should be doing (high-level direction and actions). Move it to its own method.
  • This is subjective but I feel the initialisation of argSet uses statements that are too long to be used inline like that. I would rather assign the strings and delegates to well-named variables, and then initialise the OptionSet with the variables, as right now it's a bit difficult to tell at a glance what the options are without reading every line of code.
  • Be consistent in your naming conventions - e.g. PrintError vs ShowHelp when both just write to the console - use either "Print" or "Show", not both.
  • PrintLocation and PrintWeather both take a parameter named value. Feel free to give it a more relevant name like location and weather, like that your code reads better e.g. location.City vs value.City.
  • Colour changing I feel should be kept to the printing methods, it's too low-level for Main() to call.
  • The if(days > 0) code block should be moved to its own method, again it's too low-level for Main(), and anyway it's nicer to see Main() call something like PrintWeatherForecastForNextNDays(days) (mind you that's a complete guess at what it might be doing, likely wrong) rather than having to read through that code and guess at what its purpose is.
  • Finally I'd be tempted to move all the option results into their own class, and create a new class for dealing with options so that your Main() method and class containing that method isn't cluttered with all this options/args code. I'd like to see it just do a call to something, passing in args, and either return or continue to the real program based on the result. Just an idea.
\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer, i will improve my code ;) But for me your last suggestion is unclear. Simply put city, countryCode etc. in a different class, and than call a "Parse" method in that class? \$\endgroup\$ – R3turnz Aug 29 '16 at 13:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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