code it better...

[eliasbyvasoula]

as i am a newbie to that field, i have the following code.

I would like to make it shorter and better.

Note that this code copies some cells from one worksheet to another worksheet, only at the last row that is empty.

this code must be repeated for 20 or more times. Range(*90) shows the place for the data for MIG. The range 89 show the place for ATE.

Finally, at the final code "BBB" will become "ccc", "DDD", for each type e.g bbb--> Mig, CCC-->ATE etc. Thats it.

so i am confused with ifs and thens, i tried to make something but not work (obviously). Therefore, i need your help!

  Workbooks.Open Filename:="D:\MARFIN\VASIKA ARXEIA\NAFTEMPORIKI.xls"

    
    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("AAA").Select
    Range("O90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("c65536").End(xlUp).Offset(1, 0).PasteSpecial
    
    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("AAA").Select
    Range("K90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("D65536").End(xlUp).Offset(1, 0).PasteSpecial

    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("AAA").Select
    Range("L90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("E65536").End(xlUp).Offset(1, 0).PasteSpecial
        
    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("AAA").Select
    Range("M90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("F65536").End(xlUp).Offset(1, 0).PasteSpecial

    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("AAA").Select
    Range("C90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("G5536").End(xlUp).Offset(1, 0).PasteSpecial

    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("AAA").Select
    Range("N90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("H5536").End(xlUp).Offset(1, 0).PasteSpecial
    
    Windows("NAFTEMPORIKI.xls").Activate
    Sheets("BBB").Select
    Range("H212").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("I65536").End(xlUp).Offset(1, 0).PasteSpecial xlPasteValues
    
    
    Cells.Replace what:=Chr(44), Replacement:=Chr(46)


End Sub

[Nightwalker83]

Can you please place [code][/code] tags around your code so that it makes it easier to read. Thank you!

[si_the_geek]

Welcome to VBForums

There are a few ways to make the code shorter and better.

The first is to use variables for the workbooks, as it makes the Windows() lines quicker and safer. To do that you would change the Workbooks.Open line to this:

Dim objBookSource as Workbook, objBookTarget as Workbook
  Set objBookSource = Workbooks.Open (Filename:="D:\MARFIN\VASIKA ARXEIA\NAFTEMPORIKI.xls")
  Set objBookTarget = WorkBooks("OLA 20.xlsx")

...and add this just before the End Sub:

  Set objBookSource = Nothing
  Set objBookTarget = Nothing

You can then change the Windows() lines to these:

  objBookSource.Activate
  objBookTarget.Activate

(you can use other names instead of objBookSource and objBookTarget if you like, but they are reasonably short and clear)


The next step is to not Activate or Select anything unless you absolutely need to, as they are slow and unreliable. In terms of lines like these:

    objBookSource.Activate
    Sheets("AAA").Select
    Range("O90").Select

...you can simply merge the lines, as a Range is a child object of a Sheet, which is a child object of a Workbook. As such, those lines can be shortened to this:

    objBookSource.Sheets("AAA").Range("O90").Select

At this stage you are likely to get an error if you run it, but the next step fixes that.

You can now eliminate the need for Selection entirely by removing .Select from the end of one line, and Selection from the start of the next. So this:

    objBookSource.Sheets("AAA").Range("O90").Select
    Selection.Copy

..become this:

    objBookSource.Sheets("AAA").Range("O90").Copy

If you do this for all of your code, each block of 7 lines you've got will be reduced to just 2 lines. Have a go at that and show us what you end up with, as we may be able to improve it even further.

[eliasbyvasoula]

I have made some changes in order to make it shorter. And this is the result... i will try your suggestions.

However, i found to have a problem with that line.
i would like to RePLACE all data inside the same workbook BUT this cxommand replaces only data that exists in the active sheet. As i went through web, there is not an easy right-way solution. right;

Cells.Replace what:=Chr(44), Replacement:=Chr(46)

Sub S3()

    Workbooks.Open Filename:="D:\MARFIN\VASIKA ARXEIA\NAFTEMPORIKI.xls"

    Windows("NAFT.xls").Activate
    Sheets("AAA").Select
    Cells.Find(what:="MRFr.AT", After:=ActiveCell, LookIn:=xlFormulas, _
    LookAt:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
    MatchCase:=False, SearchFormat:=False).Activate
    Range("S90:Y90").Select
    Selection.Copy
    Windows("OLA 20.xlsx").Activate
    Sheets("MIG").Select
    Range("c65536").End(xlUp).Offset(1, 0).PasteSpecial xlPasteValues
        
    Cells.Replace what:=Chr(44), Replacement:=Chr(46)




End Sub

[si_the_geek]

Just like Range, Cells is a child object of a sheet.

If you don't specify a parent object (in this case a sheet) then Excel will assume that you want whichever is Active/Selected at the time.

In order to do something on each sheet, you will need to use a loop for the Workbook.Sheets collection, eg:

Dim objSheet as WorkSheet
  For Each objSheet In objBookSource.Sheets
    MsgBox objSheet.Name
  Next

[eliasbyvasoula]

Thanxs, i am trying to do it!

However, now, i have a serious problem!
I use a web query, a very simple one e.g.

1
http://www.naftemporiki.gr/markets/excel_intraquote.asp?id=["id","Doste to symbolo poy thelete px NAYr.AT : "]

I use this to an excel sheet. each time that is opened is renewed. It is giving intraday quotes. I am multiplying stocks buyed*price to find the VALUE.

Note that, the number of ROWS that will be filled by the web query are not standards (e.g. one time 100, another time 320)

HOWEVER, i have that problem. the prices are taken in the simplest way:

A            B           C
PRICE     VOL      =A1*B1
1.01       120      =A2*B2
1.05       2300     =A3*B3
1.02       1300     =A4*B4
.
.
.
.
.

Values are given in a very nice way, however, when i close the excel file, and i am opening again, i renew the web query. Then, abnormality is appearring:

A             B           C
PRICE     VOL        =A1*B1
1.01       120        =A2*B2
1.05       2300       =A3*B3
1.02       1300      =A100*B100
1.04        300       =A101*B101
1.03        340       =A102*B102
1.07        500       =A103*B103
.
.
.
.

i have understood (i think!) that the values are taken until the point that was taken before (e.g. number rows filled 120) but the next tme, if they are less than before (e.g. number rows filled 75) they will be given normally until the ROW 75 and the next VALUe will be calculated from the ROW e.g. 121!!! i thing after the point that the table taken from web query is ending.

I tried many things but nothing solve that problem! I cannot calculate the Value!!!! .
The only solution that i found i s to copy the calculation formula over and over using a macrocommand (over 20 sheets! and per day!!!)

thanks in advance!

[si_the_geek]

That is a completely different question to the one at the start of this thread, so it does not belong here - you should create a new thread for that question (in our Office Development forum).