Code, need help with optimization

[Mirelin]

Hello everybody!

I am new here, therefore, I would like to apologize if I posted my issue into the wrong forum section.

I have made a program. I called it "eggs". After certain input, for example,:
- 2
- 3

it outputs 2 crates (2 rows in each crate, 5 eggs in each row) with eggs which consist of the white and coloured egg combination. Example:

1. crate

OOOXO

OOXOO

2. crate

OOOOO

XOOOO

-----------------

However, I am wondering if the functions within my code can be optimized or written shorter in one way or another? You might need 'dat' file and 'rez' file in order to have the program fully operatable.

#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <cstdlib>
#include <time.h>
#include <ctime>

using namespace std;
int main() {

    int a, b, eachRowAverage, specificRow, randomPosition;

    srand(time(NULL) );

    ifstream dok1;
    ofstream dok2;

    dok1.open("olas.dat");
        dok1 >> a;
        dok1 >> b;
        dok1.close();

    eachRowAverage = (a * 2) / b;

        if (b > 0) {
            eachRowAverage = 1;}
        if (eachRowAverage == 0) {
            eachRowAverage = 1;}

    dok2.open("olas.rez");

        for(int crate = 1; crate <= a; crate++) {
            cout << crate << ".crate" << endl;
            dok2 << crate << ".crate" << endl;
                for (int eggrow = 0; eggrow < 2; eggrow++) {
                specificRow = 0;
                    for (int eggs = 0; eggs < 5; eggs++){
                        if (eachRowAverage == 1){
                        randomPosition = rand() % 4 + 0;}
                            else {
                            randomPosition = 6;}
                        if((specificRow < eachRowAverage) && (b > 0) ){
                            if(randomPosition != 6) {
                                if(randomPosition == eggs ){
                                cout << " X ";
                                dok2 << " X ";
                                specificRow++;
                                b--;}
                                    else{
                                    cout << " O ";
                                    dok2 << " O ";}}
                                else {
                                cout << " X ";
                                dok2 << " X ";
                                specificRow++;
                                b--;}}
                            else {
                            cout << " O ";
                            dok2 << " O ";}}

                cout << endl;
                dok2 << endl;}}
dok2.close();
return 0;
}

[doug4]

Nothing tremendously earth-shattering.

In line 26 I guess you are protecting against a negative number for b. Notice, however, that if b is 0 you will crash at line 24 (divide by zero).

You might want to loop in line 33 from 0 < a and simply add 1 when printing out. This would better match the C++ idiom and allow you to use crate as the index to an array in the future. But it is not really a big deal in this situation.

Your indentation is non-standard. Typically code is indented when inside {}. So, lines 20-22 and lines 26-29 would not typically be indented as a whole (27 and 29 would be indented only once). Additional indentation weirdness can be found in lines 33 - 63. For instance, line 37 should be indented. The else statements in line 41, 50 53 and 58 typically line up under their corresponding if statements.

Also, closing } is typically placed on the next line and lined up with the for or if or whatever that opened it. This makes it much easier to see the structure of your code and understand where the blocks are. When I look at line 63, there are 2 }} there, and it takes a while to know what blocks they are closing.

Note: There are religious wars about whether a code block should be :

    if (a == b) {
        // do work here
    }

or

    if (a == b)
    {
        // do work here
    }

Most developers have a preference. However, in both of these accepted styles, the closing } is on the following line lining up with the if.

Using the atypical indentation style that you have makes it more difficult for others to review and comment on your code.

[ne555]

I have no idea what you are trying to do, so it's kind of hard to try to optimise it

also, I agree with doug4, your identation is horrendous
ask your IDE to do it for you.


> You might need 'dat' file and 'rez' file in order to have the program fully operatable.
perhaps...

[dutch]

Forget about optimization. You need to fix absolutely everything else. Your layout is very bad. Don't invent your own style unless you never want to show your code to other people. Otherwise, use a standard style. And your algorithm is a total mess. What in the world is the program supposed to be doing? What is b for?

[jonnin]

#include <time.h>
#include <ctime>
fail. time.h is the C verson of ctime, with exactly the same stuff.

The bracket alignment is so bad I did not read the main block, but ... how slow is it, and how fast do you need it??

stuff like this is not helping you.

if (b > 0) {
eachRowAverage = 1;}
if (eachRowAverage == 0) {
eachRowAverage = 1;}

this is an awful lot of work for the result.. and later you have a triple for loop, which (without being readable, I am just eyeballing here) is often an indication of brute force.

[Niccolo]

@Mirelin,

So, you've heard a number of coding standard style issues, all related to brackets and indentation.

I'm not suggesting a style to you, but this is along the lines of what they're saying:

	
int main() 
{   int a, b, eachRowAverage, specificRow, randomPosition;

    srand(time(NULL) );

    ifstream dok1;
    ofstream dok2;

    dok1.open("olas.dat");
    dok1 >> a;
    dok1 >> b;
    dok1.close();

    eachRowAverage = (a * 2) / b;

    if (b > 0) 
      { eachRowAverage = 1;
      }

    if (eachRowAverage == 0) 
      { eachRowAverage = 1;
      }

    dok2.open("olas.rez");

    for(int crate = 1; crate <= a; crate++) 
      { cout << crate << ".crate" << endl;
        dok2 << crate << ".crate" << endl;

        for (int eggrow = 0; eggrow < 2; eggrow++) 
          { specificRow = 0;
            for (int eggs = 0; eggs < 5; eggs++)
              {
               if (eachRowAverage == 1)
                    { randomPosition = rand() % 4 + 0;
                    }
               else { randomPosition = 6;
                    }

               if((specificRow < eachRowAverage) && (b > 0) )
                    { if(randomPosition != 6) 
                          { if(randomPosition == eggs )
                                { cout << " X ";
                                  dok2 << " X ";
                                  specificRow++;
                                  b--;
                                }
                            else{ cout << " O ";
                                  dok2 << " O ";
                                }
                          }
                     else { cout << " X ";
                            dok2 << " X ";
                            specificRow++;
                            b--;
                          }
                    }
               else { cout << " O ";
                      dok2 << " O ";
                    }
              }

            cout << endl;
            dok2 << endl;
          }
      }

 dok2.close();
 return 0;
}

Coding standards include a number of instructions on idioms, formatting and naming conventions.

I realize it isn't the focus of attention in much early study, but perhaps you can see how more comprehensible the code is merely by having code grouped and indented by some association with the loops and/or logic (if/else).

That said, here's one thing that strike me:

I am wondering if the functions within my code can be optimized

Since you haven't written any functions (only int main()), this is, at first, a puzzling request.

My response is group some of this code into functions.

Linus Torvalds, from whom I don't take too many recommendations (just the OS code he chairs), I paraphrase a quote of his: "If you have more than 3 indents in your code, you need to fix your code".

He may have several meanings, but a simple interpretation is that by the time you have a 3rd indent, you've probably defined something that should be a function and not indented code.

I sense that optimization may mean something to you we don't recognize. To us, optimization is about increasing speed, reducing memory requirements - that kind of thing. I think you probably aren't referencing that.

Structure is what is needed here, and I think that may be what you're describing here, not optimization as we know it. This code sends a few characters at a time to a stream, which isn't high performance by itself, so optimization is probably not a goal. This is also not likely a challenge to a modern CPU.

There are also more basic issues. What if b comes from the file as zero? That will cause a division by zero (crash) just after you close dok1.close();

You test for b>0 later, after the division by zero crash might have already happened. Think how to structure that so it makes more sense.

It seems the only reason for reading from olas.dat is to feed input to integers a & b. Perhaps this is best a concept wrapped into a function that returns a and b (and so the dok1 stream isn't inside main).

There is an outer most loop that runs through crates. Perhaps everything else inside that loop is best in a function. It is an outer loop, so that's less of a performance concern (calling a function) than code inside the most interior loop. It would, however, show us that there's a loop running through crates.

We had to deduce that by looking to see where the loop ended, way, way down the page.

You're also duplicating the output at every stream statement for cout and dok2. Every time you find a pattern repeated like that you're probably doing too much work, and should make a function that does that.

If you know how to make classes, then perhaps you need a class that does this duplicate output for everything you're streaming.

Maybe that's just a non-member function taking dok2 and your string (cout is global).

If cout duplicates are just for debugging/testing, such a method makes that optional, at will.


Replicate that thinking throughout the code and see where you land.

[Mirelin]

I would like to express my sincerious gratitude for all your replies, guys. I must admit, from reading all your comments; I have learnt much more than from my beginners programming lectures at the uni.

1) doug4 and Nicollo, thanks for your suggestions. I couldn't even imagine that the code itself becomes so much easier to read after correct conventions.

2) dutch, ne555 and jonnin. I am very sorry for confusing you this much. That was not my intention, I am sorry.

The purpose of the program is to create 2 crates (each with 10 eggs which look like - O) and to distribute 3 white eggs among these crates, which look like - X

Consequently, I input the amount of eggs through the 'olas.dat' file. In my dat file I have the following numbers written down, each in a new line

2
3

2 - stands for amount of crates and 3 stands for amount of coloured eggs to be distributed amongst them.

As a result, in my output 'olas.rez' file, I am receiving the following result:

1.crate
X O O O O
O O O O O
2.crate
O O X O O
O O O X O

As you can see from the output, the program works, and I asked whether I can make it any better. For example, by rewriting certain 'if' or 'for if' cycle or maybe replacing it with something else? Thanks, once again, to everybody for telling me about the brackets and spacing. That's what I also needed pretty much.

Regarding the speed, I don't really need the speed; as it has been said, the code is pretty simple and not hardware demanding, therefore, it is not necessary. However, I learn so much better when I know other ways how I can reach the same result by, for example, modifying certain part of the code.

I fixed the spacing, is it much better now?

#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <cstdlib>
#include <ctime>

using namespace std;
int main()

    // a = crates || b = coloured eggs || hence - ' O ' stands for white eggs (the majority), ' X ' stands for the coloured eggs
{   int a, b, eachRowAverage, specificRow, randomPosition;

    srand(time(NULL) );

    ifstream dok1;
    ofstream dok2;

    dok1.open("olas.dat");
    dok1 >> a;
    dok1 >> b;
    dok1.close();

    eachRowAverage = (a * 2) / b;

    if (b > 0)
      { eachRowAverage = 1;
      }

    if (eachRowAverage == 0)
      { eachRowAverage = 1;
      }

    dok2.open("olas.rez");

    for(int crate = 1; crate <= a; crate++)
      { cout << crate << ".crate" << endl;
        dok2 << crate << ".crate" << endl;

        for (int eggrow = 0; eggrow < 2; eggrow++)
          { specificRow = 0;
            for (int eggs = 0; eggs < 5; eggs++)
              {
               if (eachRowAverage == 1)
                    { randomPosition = rand() % 4 + 0;
                    }
               else { randomPosition = 6;
                    }
               if((specificRow < eachRowAverage) && (b > 0) )
                    { if(randomPosition != 6)
                          { if(randomPosition == eggs )
                                { cout << " X ";
                                  dok2 << " X ";
                                  specificRow++;
                                  b--;
                                }
                            else{ cout << " O ";
                                  dok2 << " O ";
                                }
                          }
                    else { cout << " X ";
                           dok2 << " X ";
                           specificRow++;
                           b--;
                         }
                    }
               else { cout << " O ";
                      dok2 << " O ";
                    }
              }

            cout << endl;
            dok2 << endl;
          }
      }

dok2.close();
return 0;
}

Addition,
'a' variable in the code stands for the amount of crates. (Each crate has 2 rows, each row has 5 eggs)
'b' = variable for the coloured eggs to be distributed amongst the both crates

[Niccolo]

The format looks better, now what comes next depends on a few factors.

The first one, though, is this:

    eachRowAverage = (a * 2) / b;

    if (b > 0)
      { eachRowAverage = 1;
      }

    if (eachRowAverage == 0)
      { eachRowAverage = 1;
      }

The only way eachRowAverage could be other than 1 is if b were negative. If b were zero, this crashes.

I mentioned it earlier, but that was a small part of the larger subject.

I don't know exactly what range b might take, but I assume it shouldn't be negative.

What's worse, though, is that eachRowAverage is not initialized. It will contain garbage on a release build, might be debugging markers in a debug build, so if b were zero, eachRowAverage would probably be something wild and unexpected.

This means you should initialize the integers you allocate where you allocate them. As a habit, this is important long term. There are rare circumstances where you choose to leave values uninitialized, but it is in high performance situations beyond your study at present.

If I assumed eachRowAverage were initialized to zero, perhaps:

    if ( b > 0 )
      { eachRowAverage = (a * 2) / b;
      }

    if (eachRowAverage == 0)
      { eachRowAverage = 1;
      }

However, should the program proceed if b <= 0 ?

Deal with that and contemplate an answer to my next question.

The "depends on" I mentioned at top is this: Do you know how to make classes?

How to deal with the organization of the rest of the code depends on whether you know them and can use them in this assignment.

[Mirelin]

Unfortunately, at the uni we do everything in the main section. We've never been taught to make something outside of that :/

[Niccolo]

Well, then that's out.

For now, anyway.

How long has the class gone without having at least functions outside of main (more than 6 months)?

I would hope you might notice you have at least used objects. It may not be clear, but ifstream, for example, is a class that you've consumed here.

[jonnin]

that is much better.

some ideas on cleanup.

randomPosition = rand() % 4 + 0; //why do you add zero? It has no effect.

you can do multiples in for loops:
specificRow = 0;
for (int eggs = 0; eggs < 5; eggs++)

can be written
for (specificRow = 0, int eggs = 0; eggs < 5; eggs++)

magic numbers are a bad plan.
for (int eggs = 0; eggs < 5; eggs++) //why 5? what is 5?
make a named constant that tells me what 5 is, even if its just 'maxeggs' eg
const int maxeggs = 5;
for (int eggs = 0; eggs < maxeggs; eggs++) and do the same thing for your other magic numbers -- 2? 6? what are they?

it looks like you could remove all the cout statements and at the very end print the file to screen. it would declutter it somewhat, its not necessary. both dos and unix support print to screen from text file. system("type blah.txt") or system("cat blah.txt") where cat is unix (of course, since the name of it tells normal humans nothing about what it does).

it seems like a little creativity could boil 40-70 down so you don't repeat blocks of code. I could be wrong. But it looks like you could. It may be confusing if you do that -- often condensing logic is less clear.

[doug4]

The indentation is better. However, with all due respect to @Niccolo, this style is still fairly obscure. I showed the 2 most common styles in my previous post. The main difference--the one that causes the most arguments among developers--revolves around whether to place the opening curly brace at the end of the opening line or at the beginning of the next line. In both cases the closing curly brace lines up with the line that opens it.

I'm sure that this style is quite comfortable to @Niccolo and he uses it quite efficiently, but the style that you borrowed from him has 2 issues that I find non-starters.

1. You determine the placement of an opening curly brace after an 'if' based on whether or not you have a subsequent 'else'. Compare lines 27 and 45 in your code. What happens if you decide to add or remove an else statement later on? Do you need to reformat an entire block of code? Indentation level should be fixed independently of whether an else statement follows.

2. The sub-indentation makes it hard to see where the curly braces are lined up. For instance, it is hard to tell whether the closing curly in line 70 is associated with the 'for' in line 40 or the 'for' in line 42.

Yes, this style is much better than the style you began with. However, it still looks awkward compared to most C++ code that I have seen in production and on the internet.

I would recommend either of the following:

    if (eachRowAverage == 0){
        eachRowAverage = 1;
    }

or

    if (eachRowAverage == 0)
    {
        eachRowAverage = 1;
    }

I (like most experienced programmers) have a strong opinion about which of these 2 styles is better, but both are main stream and won't cause an experienced programmer to blink. When you get a job as a programmer, your company will probably give you a coding standard document that will tell you, among other things, where to put your braces. Until then, you should get comfortable with a common style.

[Niccolo]

@doug4, the real bottom line for the OP is that there are simply too many indentations and bracketed code here, but he can't use functions or objects.

Without functions, at the very least, I see little hope to do much.

...and yes, I work in my own little world, come from a time before Stroustrup was known and coding standards were hardly in existence. Then, too, I've always been the boss. I started my own firm decades ago and set the particulars as a result, though I don't object to the second standard you demonstrate. The first one is, of late, called "Stroustrup" formatting, ever since he wrote that it is his own preference, but it is the one thing he does I disagree with. I really don't like the open bracket out of line with the close that relates to it.

[jonnin]

Ive always thought of the misaligned bracket stuff as 'book' code.
That is, its only saving grace is that when you are printing a text book with programs in it, you can save a great many pages (printing cost!) and keep the price of the book down by condensing.

electronic code, with modern hard drives being well over a megabyte in size and stuff, saving a few end of line statements serves no purpose. Ive had a few coders argue that they need more code on the page at any given time, see more of what is going on, but in that case I argue we should write code in paragraph form. They then tell me we need whitespace and proper indentation. Some people... can't be pleased :P

[Niccolo]

Some people... can't be pleased :P

TRUTH!

Indeed, Stroustrup wrote that his preference for the open bracket "in line" (not on a new line, aligned with it's closing bracket) is about vertical screen space for him.

He also discussed the fact that creating one's own custom standard is more important than following some presumed "industry standard" (which doesn't really exist), noting that there will be mixed standards no matter what one does, unless the code base is entirely from one team in one organization.

I've consulted for teams in their 3rd "generation", where two previous teams have retired, and there are no less than 3 different coding standards implemented throughout their code (products that go back over 15 years).

I also notice that the tendency is for earlier "generations", representing a well tuned and happy, productive team, inevitably give way to a "corporate" feel, some two generations later, in which adherence to coding standards is enforced at existential penalty, but actual functional content is not, and progress has nearly stalled.

Such members find this style abhorant:

class VirtualFunctor
{
 private:
            bool  called;
   volatile bool  cancelRequested;

 public:
                  VirtualFunctor() : called( false ), cancelRequested( false ) {}
       virtual ~  VirtualFunctor() {}

    virtual void  operator ()()=0;

     inline bool  WasCalled()      { return called; }
     inline void  Cancel()         { cancelRequested = true; }               

    virtual void  CallFailed();
    virtual void  CallSucceeded();
};

This style involves the use of a typical but "advanced" text editor (not Vi or notepad). The focus of attention is a firm vertical line separating NAMES from return types (or type declarations in the invariant).

This idea, which is obviously not popular, can be demonstrated to aid productivity because it is obvious what the names of the functions or member variables are at a glance. The author calls it "movie credits" format, and it was motivated by astigmatism (where focus is limited to a few letters of text). .

It may be trivial in reality, but some of these notions are more important than others (hence at least 2 mainstream notions about bracket indentation).

It is more important to get things like the choice between int& a, int &a or int & a, and stick with it at least for a translation unit. There's no hope for large projects to end up reading multiple styles in a large project that goes back years or incorporates a number of libraries by source, but it's really tough when things change in the same file or class.

There can be religious level arguments about camel case, where to put private data (at the top, at the bottom) - even where to put the main function.

My favorite uncommon "standard" is footnote commentary. It's like this:

void TimerService::QInternalAddTimerEvent( ProTimerPtr & mtp )
{
 ProLock l( threadSync );  // < 1 >

 if ( mtp )
 {
   if ( mtp->NeedsId() ) mtp->SetId( GetNextTimerId() );

   mtp->SetExpiration();   // < 2 >

   eventMap[ mtp->key ] = mtp;
 }
}

/*
< 1 >  lock to preserve Map during threaded access

< 2 >  the timer has an Id, and it's expiration has been scheduled
       it is now inserted into the eventmap
*/

This puts comments below the function/class (whatever), and does not interrupt the flow of code as it is read. As Stroustrup noted, we tend to ignore the comments when we're familiar with the code, and it gets in the way.

Coding standards vary from firm to firm, and you find some "internal" to various libraries. Good ones usually have logical reasons for their choices, and that's what actually matters most.