0
function profit(){
    int totalSales=0;
    for (int i=0; i<12;i++) // computer yearly sales
          totalSales+=montlysales[i];
   return get_profit_from_sales(totalsales);
}

So i've already determined that the 12 in the for loop should be a constant instead of just using an integer and that the montlysales should be passed as a parameter into the function so then a check can be run to see if the length of sales is equal to the integer value of months which is also twelve. I'm not sure if those are all the violations of the princples cause. I feel the last line 

return get_profit_from_sales(totalsales)

is wrong and its really bothering me cause I can't seem to figure out why it is in fact bothering me and I think I might have skipped something else.

can anyone help me verify?

AbcAeffchen
  • 14,400
  • 15
  • 47
  • 66
Bruno Salapic
  • 1
  • 1
  • 1
    I think you might be over-thinking this just a tad. The topics discussed in CC are good to keep in mind as guidelines, not to dissect a single teeny function to death. Using a named constant for months might be good for readability, though it's somewhat more excusable here to just use a literal constant since it's blatantly obvious (not really a magic number) and the 12 will never change. Asserting that `monthlysales` has a size of 12 would be a nicety if you're using a lower-level language that doesn't do safety checks, but it's in the realm of defensive programming against bugs... –  May 25 '15 at 08:35
  • ... rather than error-handling (against exceptional user input), and may be redundant if your language/lib already performs such checks. About the last part being bothersome, could you add some more details about what `get_profit_from_sales` does? One thing that seems obvious is that you're relying on globals a lot, and it's why this `get_profit_from_sales` doesn't make immediate sense to me (I can't see what states it depends on or what it does if you just pass the number you computed from `profit` to it). –  May 25 '15 at 08:37

1 Answers1

2

Summary - you should refactor out the call to another function and make this function so that it is pure and does only one thing, reducing complexity, and improving your ability to reason abstractly about your program and its correctness.

Your spidey sense is tingling and you should trust it - you are correct, but what is wrong is subtle.

Routines are best when they do one thing, and one thing only. So purity of vision is important in the prime imperative, management of complexity -- it allows our brains to be able to juggle more things because they are simpler. That is, you can just look at the function and know what it does, and you don't have to say, "it totals the sales, but it also calls another function at the end", which sort of clouds its "mission".

This is also part of functional programming and where I feel that languages have to adopt to try to implement the prime imperative spoken of in Code Complete. Functional programming has as one of its tenets, "no side effects", which is similar to "one mission" or "one purpose". What I've done to your code can also be seen as making it more functional, just inputs and outputs and nothing in or out via any other path.

Note also that function get_profit() reads like pseudocode, making it somewhat self-documenting, and you don't have to delve into any of the functions to understand what the function does or how it does it (ideally).

So, here is my idea of what I explained above (loosely coded, and not checked!).

function get_total_sales(int montlysales[]){
    int totalSales=0;
    for (int i=0; i<12;i++) // computer yearly sales
        totalSales+=montlysales[i];
    return totalSales;
}

function calc_profit(int all_sales, int all_commissions, int all_losses)(){
    // your calculations here
    int profit = all_sales - all_commissions - all_losses;  // ... etc. 
    return profit;
}

function get_profit(){
    int montlysales[] = read_from_disk();
    int total_sales = get_total_sales(montlysales);
    int tot_commissions = get_tot_commissions();
    int tot_losses = get_tot_losses();
    int profit_made = calc_profit(total_sales, tot_commissions, tot_losses);
    return profit_made;
}

I read Code Complete about once a year, as coding is truly subtle at times, because it is so multidimensional. This has been very helpful to me. Regards - Stephen

Stephen Adels
  • 21
  • 3