6
\$\begingroup\$

I have a year or so of experience in developing in Java. I submitted the solution for this task, but never got the feedback that was promised. Since I still would like to get a feedback on my solution, I figured that here I will find skilled developers who wish to help me and others who may have similar questions.

Compute electricity day- and night- consumption of a client. Given client's per-hour electricity consumption for some month. Goal is to compute total day- and night- consumption. Hours are enumerated from 1 starting from beginning of month. Day hours are 7-23 at working days (8-24 at daylight saving time or summer). All other hours are night hours. Example for January 2012:

1 - 2kW (hour #1 is January 1, Sunday, 00:00 - 01:00 - NIGHT) 2 - 3kW (hour #2 is January 1, Sunday, 01:00 - 02:00 - NIGHT) 26 - 3kW (hour #26 is January 2, Monday, 01:00 - 02:00 - NIGHT) 32 - 6kW (hour #32 is January 2, Monday, 07:00 - 08:00 - DAY) ... 744 - 0kW (hour #744 is January 31, 23:00 - 24:00)

In this case, night consumption is 8 = 2+3+3 and day consumption is 6.

 /** * Used for calculating day and night tariff energy consumption for one month. Takes into account the start and end time changes of * the day and night tariff because of the day light saving time period. * This constructor uses Estonian time zone and 7-23 as day hours and 8-24 as day hours at daylight saving time. * * @param consumptionData One month's consumption data for each hour. Each hour - consumption pair has to be on a separate line. * Syntax: First the hour value, second, the separator: ' - ' and then the amount. * Hours have to be integers, and start from 1. Lines with non-integer hour values are disregarded. * Amount has to be a number value. Non-numbers are considered as zero value. * For example, if one line of data is '1 - 2.5kW', then the hour #1 is first day of a month at 00:00 - 01:00. * @param month Month (1-12) for which the data belongs to. * @param year Year for which the data belongs to. */ public MonthConsumption(final BufferedReader consumptionData, final int month, final int year) 

 /** * Used for calculating day and night tariff energy consumption for one month. Takes into account the start and end time changes of * the day and night tariff because of the day light saving time period. * * @param consumptionData One month's consumption data for each hour. Each hour has to be on a separate line. * First the hour value, then the separator ' - ' and then the amount. * Hours have to be integers, and start from 1. Lines with non-integer hour values are disregarded. * Amount can be double or integer value. * For example, if one line of data is '1 - 2kW', then the hour #1 is first day of a month at 00:00 - 01:00. * @param month Month (1-12) for which the data belongs to. * @param year Year for which the data belongs to. * @param timezone Timezone where the data was measured. Values for the timezones can be acquired * <a href="http://joda-time.sourceforge.net/timezones.html">here</a> * from the 'Canonical ID' column. This is needed to consider the daylight saving time changes. * @param dayTariffStartHour The hour of the day from which the day tariff is being measured during the standard, non-daylight saving time period. * @param dayTariffEndHour The hour of the day which is the last hour of day tariff during the standard, non-daylight saving time period. * @param dstDayTariffStartHour The hour of the day from which the day tariff is being measured during the daylight saving time period. * @param dstDayTariffEndHour The hour of the day which is the last hour of day tariff during the daylight saving time period. */ public MonthConsumption(final BufferedReader consumptionData, final int month, final int year, final String timezone, final int dayTariffStartHour, final int dayTariffEndHour, final int dstDayTariffStartHour, final int dstDayTariffEndHour) 

/** * * @return Consumption amount that the day tariff applies to. */ public Double duringDayTariff() 

 /** * * @return Consumption amount that the night tariff applies to. */ public Double duringNightTariff() @Test public void nightConsumptionIsFromFirstHourToEighthHourWhenDST() throws Exception { BufferedReader reader = new BufferedReader(new StringReader("1 - 1" + "\n" + "8 - 1" + "\n" + "24 - 10" + "\n" + "9 - 10")); MonthConsumption monthConsumption = new MonthConsumption(reader, 4, 2013); assertEquals(2, monthConsumption.duringNightTariff(), 0); } 

@Test public void testIncludedWithTheTask() throws Exception { BufferedReader reader = new BufferedReader(new StringReader("1 - 2" + "\n" + "2 - 3" + "\n" + "26 - 3" + "\n" + "32 - 6" + "\n" + "744 - 0")); MonthConsumption monthConsumption = new MonthConsumption(reader, 1, 2013); assertEquals(8, monthConsumption.duringNightTariff(), 0); assertEquals(6, monthConsumption.duringDayTariff(), 0); } 

@Test public void daylightSavingStartsOnLastSundayOfMarchInEstoniaAt01UTC_March2013() throws FileNotFoundException { String fileName = "endOfMarch2013.csv"; String fileSeparator = System.getProperty("file.separator"); final String filePath = "src" + fileSeparator + "test" + fileSeparator + "resources" + fileSeparator + fileName; BufferedReader reader = new BufferedReader(new FileReader(filePath)); MonthConsumption monthConsumption = new MonthConsumption(reader, 3, 2013); /* The "endOfMarch2013.csv" file has 50 hours, each with consumed value of 1. The 723th hour is 2013 31th March 02:00-02:59. DST starts at 03:00 (UTC 01:00). This means that the time from 3:00-3:59 is skipped. 724th hour is 2013 31th March 04:00-05:59 Therefore, 744th hour is the beginning of the next month, which will not be added.*/ assertEquals(48, monthConsumption.duringNightTariff(), 0); assertEquals(1, monthConsumption.duringDayTariff(), 0); }

Questions:

  1. Is the public interface easy to use? Is there a better way instead of BufferedReader to pass data to MonthConsumption? Do the public methods of MonthConsumption have understandable documentation?
  2. Which tests are missing? Should I create tests for private methods as well? Have you come up with a test that would give some unwanted response?
  3. Is the design of unit tests using best practices? Do the unit tests serve as an explanation of what the program does? If no, then what should I change?
  4. Are there enough/not too many comments? Are there some private methods that need comments?
  5. Do the method and variable names help in understanding and reading the code?
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Personally, I distinguish between comments and JavaDoc, the last is documentation which is non-negotiable. Can there be too many comments? Yes, absolutely. Can there be too many JavaDoc? If you do JavaDoc right, there can't be enough of it (and from reading through this, you're doing it awesome)! \$\endgroup\$ Commented Jan 10, 2014 at 9:13

2 Answers 2

Reset to default
4
\$\begingroup\$

the first thing that I noticed is that your functions do not describe an action, they are missing a verb. So when a reader comes to the function "MonthConsumption()" he has to read your comment to find out if the function calculates a mont consumption or .. or .. or consumes a month :-)

I like that you name your methos quite explicit, for my taste they are a bit too long. Good thing: in case of the @Test you exactly know the conditions for this test. But i would suggest giving shorter names.

Regarding to your comments: in "Clean Code" (by Robert C Martin) it is said that comments should not be neccessary to understand a code. However, I think comments are often neccesary to describe something in the real world.

The second function has a lot of parameters. I would suggest going for a object-based concept to avoid too much function parameters.

It is obvious that you worked and thought a lot on this example. Keep on, stay interested, a good progammer always learns :-)

\$\endgroup\$
3
  • \$\begingroup\$ I believe MonthConsumption is the name of the constructor, in which case the naming would be appropriate. \$\endgroup\$ Commented Jan 10, 2014 at 9:55
  • \$\begingroup\$ Yes, your're right! I didn't notice that the Method had no return value - so it must be the constructor. So we are missing a lot of Context / source code here in order to be able to review the code correctly. \$\endgroup\$ Commented Jan 11, 2014 at 10:17
  • \$\begingroup\$ Full source code was provided in the link. \$\endgroup\$ Commented Jan 11, 2014 at 11:29
4
\$\begingroup\$

I/O and Exception Handling

Whenever you perform I/O, you will have a possibility of IOExceptions. The code in MonthConsumption is not in a position to decide what to do if that happens. You should have the constructors declare throws IOException — unless you like the idea of giving everyone free electricity whenever your server encounters a glitch. Otherwise, your only recourse would be to throw some (wrapped) RuntimeException instead, which would be ugly.

Depending on how paranoid you are, you might also want to declare throws MalformedDataException as well.

Given that you accept a BufferedReader, I would like to know, in what state will you leave the BufferedReader after the constructor completes? Will you .close() it? (I assume not.) Would there ever be the possibility that the stream contains data for more than one month, and if so, would MonthConsumption be smart enough to stop reading just before the start of the following month? (I hope so, but I wouldn't count on undocumented behaviour.) It would be nice if you clarified these issues in the JavaDoc.

One kind of IOException of particular concern is end of file. What happens if the end of the stream is encountered after reading a complete row, but before the end of the month is reached? If we are running this code to generate a mid-month status update, that would be perfectly fine. That suggests that EOF should be treated as normal. On the other hand, if a stream prematurely terminates due to an I/O glitch while we are generating monthly invoices, a customer could get free electricity for the rest of the month. The caller should be able to distinguish between the two conditions, perhaps with a .getCurrentDateTime() method to check how far it has read.

Usability

I find public Double duringDayTariff() and public Double duringNightTariff() problematic.

I think that the naming is a bit off. "Tariff" implies that money is involved, yet in this code you are only concerned about the time periods during which a rate applies. Furthermore, you're not fetching a tariff or tariff period, but the amount of electricity consumed during a tariff period.

Avoid boxed types in interfaces. Returning a double would be more conventional. To be clear, it should be documented what the return value represents. Is it a monetary amount? An amount of electricity? In what units? Kilowatt-hours? Kilovolt-amps? Joules? Whatever unit was used in the data feed?

My recommendation here would be:

/** * Totals the electricity consumed during the month during the daytime * tariff period, in kW*h. */ public double getDaytimeConsumption() { … } /** * Totals the electricity consumed during the month during the nighttime * tariff period, in kW*h. */ public double getNighttimeConsumption() { … }

Orthogonality and Extensibility

One concern for me is the cluster of five arguments to the more complex constructor:

public MonthConsumption(…, String timezone, int dayTariffStartHour, int dayTariffEndHour, int dstDayTariffStartHour, int dstDayTariffEndHour)

This makes me uneasy for two reasons:

  1. You hard-code the idea of two tariff periods.
  2. Your code for parsing the stream and adding values is now committed to handling daylight saving time as well.

I think it would be worthwhile to decompose the problem by introducing a TariffPeriod concept. Even if the business is committed to the idea of only two tariff periods, your code would benefit from better organization.

public interface TariffPeriod { public boolean isApplicable(DateTime dt); } public class MonthConsumption { public MonthConsumption(…, String timezone, TariffPeriod... tariffPeriods) { … } /** * Totals the electricity consumed during the month during the specified * tariff period, in kW*h. The tariff period should be one of the * periods specified in the constructor. To get the consumption for * all times that fall outside any tariff period, use null for the * tariffPeriod. */ public double getConsumption(TariffPeriod tariffPeriod) { … } }

Then you can implement a TariffPeriod that includes 7h–23h during standard time and 8h—24̱h during daylight saving time. The nighttime period could be considered the "default".

Implementation Notes

Whenever you have alternate constructors, the one with more defaults should chain to the one with more explicit parameters.

Keeping a map of the hour-by-hour consumption data seems to be overkill, when all you care about are the totals for each tariff period. Just keep running totals for each tariff period as you parse.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.