Reputation:
I would love to minimize the program. Maybe putting p1-16 in one line of code, same with count and gefunden? Since my language skills are minimal I can't find the right information.
It would also be great if there was a way to minimize the if else statements in search hits pdf.
Right now I do the code by hand to add new pdfs, as in search hits pdf1 to pdf2. Any easier way would greatly help me.
function Suche(str){
p1=document.getElementById('pdf1').innerHTML;
p2=document.getElementById('pdf2').innerHTML;
p3=document.getElementById('pdf3').innerHTML;
p4=document.getElementById('pdf4').innerHTML;
p5=document.getElementById('pdf5').innerHTML;
p6=document.getElementById('pdf6').innerHTML;
p7=document.getElementById('pdf7').innerHTML;
p8=document.getElementById('pdf8').innerHTML;
p9=document.getElementById('pdf9').innerHTML;
p10=document.getElementById('pdf10').innerHTML;
p11=document.getElementById('pdf11').innerHTML;
p12=document.getElementById('pdf12').innerHTML;
p13=document.getElementById('pdf13').innerHTML;
p14=document.getElementById('pdf14').innerHTML;
p15=document.getElementById('pdf15').innerHTML;
p16=document.getElementById('pdf16').innerHTML;
p17=document.getElementById('pdf17').innerHTML;
gefunden1=0;
gefunden2=0;
gefunden3=0;
gefunden4=0;
gefunden5=0;
gefunden6=0;
gefunden7=0;
gefunden8=0;
gefunden9=0;
gefunden10=0;
gefunden11=0;
gefunden12=0;
gefunden13=0;
gefunden14=0;
gefunden15=0;
gefunden16=0;
gefunden17=0;
count1=0;
count2=0;
count3=0;
count4=0;
count5=0;
count6=0;
count7=0;
count8=0;
count9=0;
count10=0;
count11=0;
count12=0;
count13=0;
count14=0;
count15=0;
count16=0;
count17=0;
searchstring=str;
//Search Hits PDF1
endsearch=p1.length;
weiter=1;
if(p1.indexOf(str)>-1){
gefunden1=1;
pos1=p1.indexOf(str)+searchstring.length;
count1=count1+1;}
else{weiter=0;}
for(i = 1; i <=10; i++){
if(weiter==1){
if(p1.indexOf(str,pos1)>-1){
pos2=p1.indexOf(str,pos1)+searchstring.length;
if (pos2<=endsearch){
if(count1<10){
count1=count1+1;
pos1=pos2;}
else{
count1="Mehr als 10";
pos1=pos2;}}
else{
weiter=0;}}
else{
weiter=0;}}}
//Search Hits Pdf2
endsearch=p2.length;
weiter=1;
if(p2.indexOf(str)>-1){
gefunden2=1;
pos1=p2.indexOf(str)+searchstring.length;
count2=count2+1;}
else{weiter=0;}
for(i = 1; i <=10; i++){
if(weiter==1){
if(p2.indexOf(str,pos1)>-1){
pos2=p2.indexOf(str,pos1)+searchstring.length;
if (pos2<=endsearch){
if(count1<10){
count2=count2+1;
pos1=pos2;}
else{
count2="Mehr als 10";
pos1=pos2;}}
else{
weiter=0;}}
else{
weiter=0;}}}
and so on....
Upvotes: 1
Views: 270
Reputation: 40872
I agree that this should be on code review. But you have a working code and ask how to make it better. So here you go.
replace all those variables that have the format variableN
with an array. As soon as you have such a naming format you most of the time either want to use an array or change the name.
And you definitely want to clean up that function that searches for the occurrence of the given string.
Always define variables using const
or let
. And add comments to the code.
If something reflects a boolean, then use one instead of 0
or 1
.
Make use of comments, that will also help others when looking at your code (and it also helps you if you look at your code after a while).
Use variables instead of magic numbers like 10
.
and even if your preferred language is not the one used of the programming language, you should stick with the one the programming language use.
So here is a reworked version of your code:
// use an options parameter to make the function more flexible
function search(str , options) {
// destructuring the options into variables
const {maxCount, pdfCount} = options;
const items = [];
for (let i = 1; i <= pdfCount; i++) {
items.push({
p: document.getElementById(`pdf${i}`).innerHTML,
found: false,
count: 0
})
}
items.forEach(item => {
let count = 0;
let currentPosition = 0; // position where to start searching
let foundAtPosition;
// do-while loop to do at least one search
do {
foundAtPosition = item.p.indexOf(str, currentPosition);
// check if we found an occurence
if (foundAtPosition != -1) {
// increase the count
count++;
// set the current position after the found occurence
currentPosition = foundAtPosition + str.length;
}
// if we found more then maxCount we can leave the loop
if (count > maxCount) {
break;
}
// only continue the loop when something was found
// you could move "count > maxCount" to the while condition
// but the main purpose of the while loop is to iterate over the content
} while (foundAtPosition != -1);
// do the composing the information to be set for the item after the for loop,
// that makes many things clearer as it is not part of the searching process
// set found to true or false by checking if count is larger then 0
item.found = count > 0;
if (count > maxCount) {
item.count = `Mehr als ${maxCount}`;
} else {
item.count = count;
}
})
return items;
}
console.dir(search('hey', {maxCount: 10, pdfCount: 3}))
<div id="pdf1">
heyheyaaheyaaahey
</div>
<div id="pdf2">
heyheyaaheyaaahey
heyheyaaheyaaahey
heyheyaaheyaaahey
</div>
<div id="pdf3">
foo
</div>
You could also utelize str.split([separator[, limit]])
as mention here How to count string occurrence in string? and utilize the limit
function.
But if you do that you really need to document the item.p.split(str, maxCount+2).length - 1
construct because that's hard to understand otherwise.
// use an options parameter to make the function more flexible
function search(str , options) {
// destructuring the options into variables
const {maxCount, pdfCount} = options;
const items = [];
for (let i = 1; i <= pdfCount; i++) {
items.push({
p: document.getElementById(`pdf${i}`).innerHTML,
found: false,
count: 0
})
}
items.forEach(item => {
// use maxCount + 2 to figure out if we have more then max count subtract 1 from length to get the actual count
const count = item.p.split(str, maxCount+2).length - 1
// set found to true or false by checking if count is larger then 0
item.found = count > 0;
if (count > maxCount) {
item.count = `Mehr als ${maxCount}`;
} else {
item.count = count;
}
})
return items;
}
console.dir(search('hey', {maxCount: 10, pdfCount: 3}))
<div id="pdf1">
heyheyaaheyaaahey
</div>
<div id="pdf2">
heyheyaaheyaaahey
heyheyaaheyaaahey
heyheyaaheyaaahey
</div>
<div id="pdf3">
foo
</div>
Upvotes: 0
Reputation: 101
Why not use an array called p?
const p = []
for (let i=1; i<18; i++) {
p.push(document.getElementById(`pdf${i}`).innerHTML)
}
You can do the same for gefunden and count. The rest of your code, if repetitive, could go in a function and be called in another for loop.
Upvotes: 4
Reputation: 109
You can use arrays.
Arrays, in a simple way, put lots of info into one variable. For example:
var this_is_an_array=[0,1,2,3,4,5]
Arrays count from 0 and onwards, so do take note to start counting from 0
More in detail at w3 schools: https://www.w3schools.com/js/js_arrays.asp
Upvotes: 0