4
\$\begingroup\$

I have a program that has one job: to determine how many unique numbers there are in a text file. The text file contains a string of numbers created by this other program named Randomizer.py:

# Import random module
import random

# Variables for holding and transporting data
values = []
val_str = ""


def randomize():
    # Create original numbers
    ran_int = random.randint(500, 699)
    for num in range(0, ran_int):
        values.append(num)

    # Create duplicates
    ran_int_two = random.randint(250, 300)
    for duplicate in range(0, ran_int_two):
        val = random.choice(values)
        values.append(val)

    # Shuffle values... literally
    random.shuffle(values)

    list_transfer()


def list_transfer():
    global val_str
    # Place values in file
    text_file = open("text_input", "w")
    for index in range(0, len(values)):
        val_str += (str(values[index]) + " ")
    text_file.write(repr(val_str))
    text_file.close()

It creates a random count of unique numbers then creates a random count of duplicate numbers. All get sent to a text file called text_file. Example output in file (3,630 chars long):

'206 428 186 321 648 612 171 418 447 565 355 430 250 567 314 526 360 151 171 595 452 397 195 416 54 604 485 335 460 35 633 479 555 39 557 574 320 610 20 127 64 556 552 230 56 234 336 207 548 634 244 507 247 332 350 564 241 586 169 94 277 58 332 42 607 65 419 558 27 144 216 311 229 57 179 152 341 333 156 448 597 580 66 641 453 360 376 178 385 595 296 197 391 505 110 563 275 597 362 560 266 396 531 510 260 518 57 190 100 293 553 501 623 256 579 226 63 334 70 647 294 492 307 592 528 488 436 591 464 626 633 541 211 585 264 398 283 577 366 254 491 220 261 626 469 91 636 290 550 598 315 476 90 533 583 227 14 627 305 494 466 262 362 183 472 451 468 65 393 450 603 609 362 362 322 356 118 575 478 20 335 206 323 579 21 261 109 380 89 508 533 516 484 44 473 168 335 31 604 261 509 547 464 462 80 93 383 402 103 565 48 218 49 155 135 143 547 357 88 277 586 462 536 630 493 425 35 36 303 39 611 22 50 551 408 223 445 540 258 265 38 3 84 639 601 614 99 352 188 182 330 512 503 8 562 619 294 629 591 259 457 454 546 373 284 15 210 499 372 481 271 194 506 547 590 243 426 359 620 53 281 402 399 356 504 447 418 544 307 212 113 461 647 589 587 602 175 586 107 198 422 273 638 524 197 483 290 588 578 394 257 197 17 405 133 83 347 401 616 42 367 2 511 204 267 584 469 552 463 35 544 308 606 547 85 51 583 238 161 170 120 388 155 524 608 205 617 180 643 116 358 622 5 306 101 460 433 278 381 221 24 254 565 485 465 561 550 428 87 382 228 326 210 569 249 545 199 634 649 123 68 483 273 513 490 525 452 292 33 157 34 132 1 70 286 144 484 523 338 619 496 128 174 646 425 122 95 152 561 602 184 139 526 26 342 154 354 576 441 173 537 202 530 309 66 615 340 166 240 201 74 197 83 474 47 37 298 397 331 159 245 289 419 438 363 349 441 447 480 71 197 218 97 570 225 339 115 150 319 445 239 82 475 594 550 639 219 254 637 337 145 22 263 482 415 546 384 224 87 312 467 134 499 648 163 539 572 648 343 232 165 79 485 302 313 186 59 125 362 197 180 326 450 549 94 354 517 337 444 142 177 307 628 38 231 470 433 10 540 301 412 582 148 16 272 268 143 344 431 136 394 215 102 255 242 481 338 387 379 153 237 150 146 104 108 231 52 62 504 477 446 150 291 137 413 559 241 160 531 32 581 69 348 462 322 211 328 471 66 210 106 95 137 519 72 378 64 117 404 46 241 547 176 187 579 556 264 141 29 330 279 647 316 43 331 69 317 346 315 111 437 640 420 81 454 495 233 569 22 583 421 206 241 216 188 459 506 456 276 253 138 424 426 527 6 108 477 482 535 486 3 156 172 595 538 119 241 328 233 335 450 451 491 297 629 382 338 208 363 635 248 162 327 642 214 417 418 0 167 193 428 60 431 331 521 419 270 326 209 375 203 522 429 188 300 370 303 192 518 361 123 631 322 380 647 140 600 432 73 443 608 435 502 571 329 602 461 400 235 130 269 280 188 41 593 158 101 287 75 13 353 371 534 511 335 143 529 412 425 251 191 390 211 282 222 555 514 638 78 18 76 124 45 605 428 295 436 66 537 389 353 330 189 341 280 389 213 51 439 365 385 117 596 406 489 484 457 147 434 123 455 181 442 351 444 624 606 128 467 648 626 196 368 204 287 246 509 376 77 579 356 407 242 377 394 12 271 236 403 47 443 645 131 552 325 310 55 116 210 397 626 28 51 618 615 334 497 169 613 428 324 234 544 24 129 566 524 98 286 128 596 197 96 452 423 420 621 395 252 25 382 359 410 386 411 288 86 539 449 104 644 646 369 520 114 554 364 631 353 626 471 440 61 424 590 543 211 515 205 349 30 642 81 304 392 595 596 189 559 414 217 294 150 274 67 356 285 301 126 498 487 200 254 92 427 458 12 448 599 573 299 95 625 398 468 519 409 542 28 339 541 596 121 39 629 34 40 95 42 557 515 631 185 11 574 489 141 389 149 4 500 9 568 162 241 345 597 374 512 289 318 105 632 7 234 19 23 55 112 164 532 457 617 '

This data is then used by my program named Unique_Values.py:

import time
import Randomizer

# Create new data values
Randomizer.randomize()

# Variables holding information and results
unique_values = [-2]
unique = 0
duplicates = 0
start_time = time.time()

# Get data in text file
text = open("text_input", "r")
values = text.read().split()
text.close()

# Remove unnecessary values
values[0] = values[0].replace("'", "")
values[(len(values) - 2)] = values[(len(values) - 2)].replace("'", "")
values.pop()


def isolate():
    global unique, duplicates, start_time
    for index_val in range(0, len(values)):
        for index_uni_val in range(0, len(unique_values)):
            if len(values) == 0:
                print "There were", unique, "values and", duplicates, "duplicates. There were", unique + duplicates, \
                "total values."
                end_time = time.time()
                print "This program took", end_time - start_time, "seconds to finish."
                exit()
            else:
                if int(values[index_val]) != unique_values[index_uni_val]:
                    if index_uni_val == (len(unique_values) - 1):
                        unique_values.append(int(values[index_val]))
                        del values[index_val]
                        unique += 1
                        isolate()
                    else:
                        continue
                elif int(values[index_val]) == unique_values[index_uni_val]:
                    del values[index_val]
                    duplicates += 1
                    isolate()

isolate()

Some key points about the program (Unique_Values.py):

  • The data is sent into a list called values. Some data values though are not integers and are dealt with in lines 13-16 in the last program.
  • The method used is that if the value is not seen in unique_values, it is appended into the list and the counter for unique values increases by 1.
  • unique_values needs a negative number so the program doesn't immediately stop. Negative so it can't be matched with any of the values, which are all positive.
  • The program is recursive and uses exit() when finished due to a bug where it continues looping after it is finished.

How can my program be more efficient in finding unique values, be shorter, and/or follow Pythonic standards?

\$\endgroup\$
  • \$\begingroup\$ What is wrong with collections.Counter(text.read().strip("'").split()), if you actually need the counts, or len(set(text.read().strip("'").split())) if you don't? \$\endgroup\$ – Graipher Dec 20 '16 at 18:14
  • \$\begingroup\$ @Graipher I never even heard of collections before actually \$\endgroup\$ – Anthony Pham Dec 20 '16 at 18:18
  • 1
    \$\begingroup\$ Then you should read up on it, it is a very nice standard library with stuff like Counter and defaultdict. \$\endgroup\$ – Graipher Dec 20 '16 at 18:22
6
\$\begingroup\$

Your first script can be greatly shortened. There is also no need for the global variable, at all.

import random


def randomized_values():
    # Create original numbers
    values = range(random.randint(500, 699))
    # Create duplicates
    for _ in range(random.randint(250, 300)):
        values.append(random.choice(values))
    random.shuffle(values)
    return values


def save(values, file_name):
    with open(file_name, "w") as text_file:
        text_file.write(" ".join(map(str, values)))

if __name__ == "__main__":
    values = randomized_values()
    save(values, "text_input")

Here I created the first list of values directly by casting range to a list and using the fact that range starts at 0 by default.

If you want to have exactly random.randint(250, 300) duplicates (instead of mostly duplicates and some triplicates and...), you can use values.extend(random.sample(values, random.randint(250, 300))) instead of the for loop.

I got rid of all variables that were only used once. I used the single responsibility principle, so every function only does one thing and (if possible) returns its result.

I used with..as to ensure that the file will be closed even in case of an exception. I used str.join to build the output string and got rid of the repr, because the first thing the second script does is get rid of the surrounding ''. I made the script more adaptable by making the filename a parameter.

Finally, I used if __name__ == "__main__": to allow importing parts if this module from another script.

Now to your actual script. There seems to be no need to actually save the file to the disk, it would be better to just use values = randomizer.randomized_values(). But I'm going to go with the flow of your program and keep it for now. Note that I renamed the file to be lower_case, because that is how variables should be named in Python, according to its official style-guide, PEP8.

import time
import randomizer
from collections import Counter

FILE_NAME = "text_input"


def timeit(func):
    def wrapper(*args, **kwargs):
        start = time.time()
        result = func(*args, **kwargs)
        print "The function {.__name__} took {:.2f} seconds to finish.".format(func, time.time() - start)
        return result
    return wrapper


def get_uniques(occurences):
    return sum(1 for n in occurences.itervalues() if n == 1)


@timeit
def main():
    # Create new data values
    values = randomizer.randomized_values()

    # The next two steps are actually unnecessary
    randomizer.save(values, FILE_NAME) 
    with open(FILE_NAME) as text_file:
        occurences = Counter(text_file.read().split())

    # Would also work:
    # occurences = Counter(values)

    total = len(occurences)
    unique = get_uniques(occurences)
    duplicates = total - unique

    print "There were {} unique values and {} duplicates. There were {} total values.".format(unique, duplicates, total)

if __name__ == "__main__":
    main()

Note again, that saving the values to a file and reading them again is completely unnecessary.

In order to get the number of occurences for every number, I used collections.Counter, which consumes an iterable and notes how often every element in that iterable occurs and returns a dictionary with this information.

Note that all the data cleaning has become now obsolete, since the saving just saves the values (without the surrounding ''). I again used with..as to ensure that the file is correctly closed.

In get_uniques_duplicates I used dict.itervalues, which is a generator of the values of the dict, which takes O(1) memory, in comparison to dict.values, which takes O(n) memory in Python 2.

The result is now printed using str.format, even though this was not so necessary here.

The approach for counting all unique/duplicated items is a generator expression, which means it is implemented in C internally. I use the fact that all not unique elements have duplicates.

I used a decorator to measure the time it took the function to run. Note the function here, not the whole program per se. But since I used it on main it is effectively the whole program (minus the initialization, imports and function definitions).

Edit: I just realized, I might have misunderstood your requirements. If you are looking for the number of distinct numbers in values, use this:

values = random.randomized_values()
unique = len(set(values))
total = len(values)
duplicates = total - unique

My original code would give 1 unique (as in, appears exactly once) value for values = [1, 1, 2]. This code will give 2 distinct values.

\$\endgroup\$
  • 1
    \$\begingroup\$ I was about to post this and also I think you need to map values to str - " ".join(map(str,values) \$\endgroup\$ – Gurupad Mamadapur Dec 20 '16 at 19:55
  • \$\begingroup\$ Didn't know that you had already an answer using Counter. Yours is nicer. I flushed my answer down the drain :D \$\endgroup\$ – Gurupad Mamadapur Dec 20 '16 at 20:58
  • \$\begingroup\$ @GurupadMamadapur Sorry, was still expanding it, when you wrote first. I liked yours as well, it had the same approach I started out with (iterating over the values of the Counter manually and incrementing uniques). It also had a bit more on the actual code from the OP instead of a rewrite. i would have upvoted it tomorrow when I have new votes :) \$\endgroup\$ – Graipher Dec 20 '16 at 21:00
  • \$\begingroup\$ Hey, no probs. I think there is an error on the way you are reading the values from the file. I'll edit it. \$\endgroup\$ – Gurupad Mamadapur Dec 20 '16 at 21:05
  • \$\begingroup\$ How do you use the decorator for recursive functions and only have the decorator function have only one output? \$\endgroup\$ – Anthony Pham Dec 22 '16 at 22:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.