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\$
1
\$\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\$
1
  • \$\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\$
    – user103180
    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