Iterate JSON GIF collection array to HTML file I've written a piece of code which utilizes a for loop to iterate this array in JavaScript. This is my first attempt and I wonder if there is an optimal way to do this.

Fiddle

JavaScript

var gifCollection = {
"gifs": [
  {
  "gifName" : "gif-One",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
  {
  "gifName" : "gif-Two",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
  {
  "gifName" : "gif-Three",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
  {
  "gifName" : "gif-Four",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Five",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Six",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Seven",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Eight",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Nine",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Ten",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
  }
 ]
};

var gifLink = gifCollection.gifs;
var div = document.getElementById('#vids');

for (var i in gifLink) {
  var src = gifLink[i].gifLink;
  //alert(gifLink.gifLink);
  console.log(src);
  //div.innerHTML = div.innerHTML + '';
  $("#vids").append('

');
}

HTML

There aren't many things wrong. And the code itself is surprisingly short. But this will be a really long review.

The first thing that bothers me is that you talk about JSON, but you show an object. That's fine, but you should show how you decode the JSON into that object. But, within the scope of this review, I'll refer to it as an object, which is what was presented. Not to mention that all the links point to .jpgs.

Now, lets tackle the code!

Javascript:

The indentation of that object is really off. Not to mention that you have mixed quote styles.

Each gif has the following:

{
    "gifName" : "gif-One",
    "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
}

We know those are gifs, so, why repeat gif in every single property? Using just name and link is enough.

And you don't even use the gifName anywhere! So, just drop it! And just to have there to say it is gifs, you can remove it and just use an array.

The whole object can be something like this:

var gifCollection = [
    'http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg',
    'http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg',
    ' ... '
];

Notice that you have to use " instead if you are using this in JSON! As I refered earlier, I would review it as an object.

Looking a little below, you have this line:

var div = document.getElementById('#vids');

Well, that won't work. What you have there is a CSS selector. Fear not, just remove the #. But a few lines below you have this:

//div.innerHTML = div.innerHTML + '';

It seems like you made an attempt, but it didn't worked. Since these 2 lines aren't doing anything, just remove them.

Anyway, you are using jQuery below!

Right after it, you have this block:

for (var i in gifLink) {
    var src = gifLink[i].gifLink;
    //alert(gifLink.gifLink);
    console.log(src);
    //div.innerHTML = div.innerHTML + '';
    $("#vids").append('

');
}

These comments contain old code. Just remove them! They won't kill you. And that console.log can go away as well, since it isn't useful to make the code work. It may be in debug.

But wait! You have this line:

$("#vids").append('

');

Oh no! You are creating duplicated ids! They MUST be unique! Or that will be invalid HTML. Use a class instead. Also, don't be afraid of using the id videos. No one will die over 2 bytes. And it is way more clean.

Since I've suggested to change the structure of your object, you have to use a different for loop. Applying the changes above explained, and fixing the quotes:

for (var i = 0, length = gifCollection.length; i < length; i++) {
    var gif = gifCollection[i];
    $('#videos').append('

');
}

I've kept the length there because it will speed up your code quite a lot. Speaking in 'speeding up', you are re-running $('#vids') per gif. Just store it into a variable. Like this:

var videos = $('#videos');
for (var i = 0, length = gifCollection.length; i < length; i++) {
    var gif = gifCollection[i];
    videos.append('

');
}

Now you won't be running jQuery everytime and it will be way faster!

Putting your Javascript code all together:

var gifCollection = [
    "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg",
    "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg",
    " ... "
];

var videos = $('#videos');
for (var i = 0, length = gifCollection.length; i < length; i++) {
    var gif = gifCollection[i];
    videos.append('

');
}

HTML:

The HTML is pretty basic, but there's still plenty to improve. Lets look at it on it's original state:

Alright, you have a .container which has a .video which has #vids inside? What? Videos inside a video?

What I would do:

Remove .video. It's useless there.
Change the id to videos
Give the class row to the container.
The container is the one that will control the width of your #videos.
It makes sense to set it this way.
Closing videos in the same line
Since you are using jQuery, later on you may want to use the pseudo-selector :empty, which won't work if you have newlines in it.

Applying all the changes:

And yes, it works just like the old HTML.


Texture loading in LWJGL 3 I made the following class to simpify texture loading in LWJGL 3, and modeled it after the slick-util API:

public class Texture {

    private ByteBuffer data;
    private int id;
    private int width;
    private int height;

    private ByteBuffer resizeBuffer(ByteBuffer buffer, int size) {
        ByteBuffer newBuffer = BufferUtils.createByteBuffer(size);
        buffer.flip();
        newBuffer.put(buffer);
        return newBuffer;
    }

    private ByteBuffer imageToByteBuffer(String path, int size) throws IOException {
        ByteBuffer buffer;

        URL url = Thread.currentThread().getContextClassLoader().getResource(path);
        File file = new File(url.getFile());

        if (file.isFile()) {
            FileInputStream stream = new FileInputStream(file);
            FileChannel channel = stream.getChannel();
            buffer = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());

            channel.close();
            stream.close();
        } else {
            buffer = BufferUtils.createByteBuffer(size);
            InputStream source = url.openStream();

            if (source == null) {
                throw new FileNotFoundException(path);
            }

            try {
                ReadableByteChannel channel = Channels.newChannel(source);

                try {
                    while (true) {
                        int bytes = channel.read(buffer);

                        if (bytes == -1) {
                            break;
                        }

                        if (buffer.remaining() == 0) {
                            buffer = resizeBuffer(buffer, buffer.capacity() * 2);
                        }
                    }

                    buffer.flip();
                } finally {
                    channel.close();
                }
            } finally {
                source.close();
            }
        }

        return buffer;
    }

    public Texture(String path) throws IOException {
        IntBuffer width = BufferUtils.createIntBuffer(1);
        IntBuffer height = BufferUtils.createIntBuffer(1);
        IntBuffer components = BufferUtils.createIntBuffer(1);
        data = STBImage.stbi_load_from_memory(imageToByteBuffer(path, 1024), width, height, components, 4);
        id = GL11.glGenTextures();

        GL11.glBindTexture(GL11.GL_TEXTURE_2D, id);
        GL11.glTexParameteri(GL11.GL_TEXTURE_2D, GL11.GL_TEXTURE_MAG_FILTER, GL11.GL_LINEAR);
        GL11.glTexParameteri(GL11.GL_TEXTURE_2D, GL11.GL_TEXTURE_MIN_FILTER, GL11.GL_LINEAR_MIPMAP_LINEAR);
        GL11.glTexParameteri(GL11.GL_TEXTURE_2D, GL11.GL_TEXTURE_WRAP_S, GL12.GL_CLAMP_TO_EDGE);
        GL11.glTexParameteri(GL11.GL_TEXTURE_2D, GL11.GL_TEXTURE_WRAP_T, GL12.GL_CLAMP_TO_EDGE);
        GL11.glTexParameteri(GL11.GL_TEXTURE_2D, GL14.GL_GENERATE_MIPMAP, GL11.GL_TRUE);

        this.width = width.get(0);
        this.height = height.get(0);

        GL11.glTexImage2D(GL11.GL_TEXTURE_2D, 0, GL11.GL_RGBA8, this.width, this.height, 0, GL11.GL_RGBA, GL11.GL_UNSIGNED_BYTE, data);
        GL11.glBindTexture(GL11.GL_TEXTURE_2D, 0);
        STBImage.stbi_image_free(data);
    }

    public int getWidth() {
        return width;
    }

    public int getHeight() {
        return height;
    }

    public int getID() {
        return id;
    }

    public ByteBuffer getByteBuffer() {
        return data;
    }

    public byte[] getByteArray() {
        return data.array();
    }

    public void bind() {
        GL11.glEnable(GL11.GL_TEXTURE_2D);
        GL11.glBindTexture(GL11.GL_TEXTURE_2D, id);
    }

    public void release() {
        GL11.glDisable(GL11.GL_TEXTURE_2D);
    }
}

I use it as follows:

bg.bind();
GL11.glBegin(GL11.GL_QUADS);

GL11.glTexCoord2f(0F, 0F);
GL11.glVertex2f(-1F, 1F);

GL11.glTexCoord2f(1F, 0F);
GL11.glVertex2f(1F, 1F);

GL11.glTexCoord2f(1F, 1F);
GL11.glVertex2f(1F, -1F);

GL11.glTexCoord2f(0F, 1F);
GL11.glVertex2f(-1F, -1F);

GL11.glEnd();
bg.release();

where bg is a declared Texture that throws no errors.

This is a pretty straightforward and readable implementation, and I see how it can be used. Here are some suggestions:
Use Modern OpenGL
It looks like you're using OpenGL 1.x. That's really old OpenGL. You should be using OpenGL 3 or later, really. It's supported on all major platforms. It's not too difficult to learn if you already know OpenGL 1 and it offers a lot of benefits.
Keep Track of Your Texture Target
While a texture ID is certainly an important piece of information to keep around, it's not the only piece you need. Almost all texture-related functions in OpenGL require a texture target instead of or in addition to a texture ID. If you don't keep it around you'll either end up needing it and not having it, or being forced to always use one particular texture target, which is not ideal.
Naming
While your variable names are decent, they could be even better. For example, in both resizeBuffer() and imageToByteBuffer() you have an argument named size. What units is size in? Bytes? Pixels? Vertices? You should be more explicit with a name like numBytes or size_in_bytes, or whatever's appropriate.
Also the function name imageToByteBuffer() is confusing. From the name, it seems like it's going to read a file at a path and return a byte buffer containing the pixels from the image at that path. But the second half of the function looks like it does something else. I'm not familiar enough with the Java libraries to understand when file.isFile() would return false, and what else a file would be if not a file. (I assume since it's a URL it might be a network resource?) I would recommend making 2 functions - one for reading a local file and one for reading a remote file (or whatever the else path does), with appropriate names and call them from imageToByteBuffer. Something like this:
if (file.isFile())
{
    return imageFromLocalFile(path);
}
else
{
    return imageFromRemoteFile(path, size); // or whatever it should be called
}

Also, how does a caller know what to pass in for size? Isn't that something that is discovered by reading the file?


Simple PHP Autoloader alternative Though I have heard about the magic method __autload, I still prefer to use this code:

function autoload($array)
{
    foreach ($array as $value)
    {
        switch ($value)
        {
            case 'database' :
                include ROOT_DIR.'/core/database.php';
                break ;
            case 'user' :
                include ROOT_DIR.'/core/user_class.php';
                break ;
            case 'error_handler' :
                include ROOT_DIR.'/core/error_class.php';
                break ;
            case 'template' :
                include ROOT_DIR.'/template/rain.tpl.class.php';
                break ;
            case 'email' :
                include ROOT_DIR.'/core/email_class.php';
                break ;
        }
    }
}

I'm not exactly sure if this is a good option.  Should I switch to the magic method __autoload or keep using my simple switch case?  If you propose that I need to use the magic method instead, is it due to a performance-wise factor?  If not, why are you suggesting it?

It depends if this is for OOP or procedural code.  I am assuming OOP as you use class in some of your file names.

Procedural

If this is for procedural code then it should be renamed to load as there is nothing auto about it (see the OOP review).

OOP

If this is for OOP then it is not a good option. spl_autoload_register to register a standard autoload function is better.

Your Solution

There is a hardcoded entry that needs to be maintained for each class that you want to include.  This does not scale well with hundreds or thousands of entries to look through.
If the value is not recognized you ignore it silently because you don't have a default entry in your switch statement.
You have to manually autoload the classes that you want (this takes the auto out of autoload).

Advantages of spl_autoload_register

spl_autoload_register is a true automatic loader.

// We just need one call to register an autoload function, this registers the
// autoloading for the lifetime of the program.
spl_autoload_register();
$exampleObject = new Example_Object;

// In the file autoloaded for class Example_Object:
class Example_Object
{
    // Type hinting works to load the class for Another_Object.
    public exampleMethod(Another_Object $another)
    {
        // Autoloading works here too.
        $lastObject = new Last_Object;
    }
}

The autoloader only needs to be registered once and continues to load files for classes right throughout the program.

A stack of autoload functions can be provided allowing for flexibility in the logic used to load classes.
Because files are loaded lazily only what is required is loaded no matter how much you edit the other files.
Using logic to load your class files helps you to organize them logically.  It makes your logic simpler if you name your class files logically. This helps when you have hundreds or thousands of files that have been named in a consistent manner.

What not to use an autoloader for

An autoloader is for loading the file needed to define a class. You should not use it to load functions or templates.  These should still be included manually.


Converting hex to binary in the form of string This code returns a string of binary in 4-digit forms for a given hex in the form of string
def str_bin_in_4digits(aString):
    retStr = ''
    for i in aString:
        retStr = retStr+"{0:04b}".format(int(i, 16))+" "
    return retStr.strip()

for example,
>>> str_bin_in_4digits("20AC")
0010 0000 1010 1100

The code works as expected and my concern is could it be more elegant, like faster or less memory consumption?

Review
You don't have a lot of code here to review, so this will necessarily be short.

PEP-8: The Style Guide for Python Code recommends:

snake_case for functions, variables, and parameters.  So aString should be a_string, and retVal should be ret_val.

Better parameter names

What is aString?  "Hello World" is a string, but we can't use it, because you are actually expecting a hexadecimal string.  Perhaps hex_string would be a better parameter name.
Similarly, binary_string would be more descriptive than retStr.

A '''docstring''' would be useful for the function.
Type hints would also be useful.

Alternate Implementation
Doing things character-by-character is inefficient.  It is usually much faster to let Python do the work itself with its efficient, optimized, native code functions.
Python strings formatting supports adding a comma separator between thousand groups.
>>> f"{123456789:,d}"
'123,456,789'

It also supports adding underscores between groups of 4 digits when using the binary or hexadecimal format codes:
>>> f"{548151468:_x}"
'20ac_20ac'
>>> f"{0x20AC:_b}"
'10_0000_1010_1100'

That is most of the way to what you're looking for.  Just need to turn underscores to spaces, with .replace(...) and fill with leading zeros by adding the width and 0-fill flag to the format string.
>>> f"{0x20AC:019_b}".replace('_', ' ')
'0010 0000 1010 1100'

A function using this technique could look like:
def str_bin_in_4digits(hex_string: str) -> str:
    """
    Turn a hex string into a binary string.
    In the output string, binary digits are space separated in groups of 4.

    >>> str_bin_in_4digits('20AC')
    '0010 0000 1010 1100'
    """

    value = int(hex_string, 16)
    width = len(hex_string) * 5 - 1
    bin_string = f"{value:0{width}_b}"
    return bin_string.replace('_', ' ')

if __name__ == '__main__':
    import doctest
    doctest.testmod(verbose=True)

Depending on your definition of elegant, you can one-line this:
def str_bin_in_4digits(hex_string: str) -> str:
    """
    Turn a hex string into a binary string.
    In the output string, binary digits are space separated in groups of 4.

    >>> str_bin_in_4digits('20AC')
    '0010 0000 1010 1100'
    """

    return f"{int(hex_string,16):0{len(hex_string)*5-1}_b}".replace('_', ' ')


Over-Riding User Input Here's a trivial example:

if type(length) != int:
    length = 16

as opposed to:

try:
    len_mod = (length + 33) / 32 + 1
except TypeError:
    #recover here somehow, over-ride input?
    len_mod = (16 + 33) / 32 + 1

I don't need an authoritative answer, but some opinion on this approach would be fine.

Some info about the surrounding code, as it stands now:

I don't expect anything but an int for length. I will never expect anything else.
There's no purpose in showing an error message (this is a stand-alone method)
There's no harm in suppressing the error (for now, nothing else uses this method)

With that information, is it still alright to hijack (albeit, bad) user input and suppress the resulting error by replacing it with what would have been the default value in the function?

I ask because I've never read anyone else's code where this happens, and I have wondered if there was a reason for that I'm missing.

Feel free to invent some cases where I might regret this decision, or any other points you'd like to highlight for the sake of example.

Thanks.

EDIT

Another angle: my goal is to make a useful, single serving website, quite similar to tinyurl. I tested their site using a similar bit of invalid data, and it didn't report anything back to me. It just let me be stupid:

They didn't check if my site matched a regex for syntactically valid domains, they didn't even bother mentioning that my original link was 6 characters shorter than what they gave back. I think this is a good thing, from a user's perspective. Why tell me what the site can and can't do?

I understand that this code, being for a stand-alone, single-serving application, may or may not be subject to some of the same rules as other, more modular functions are. Given the circumstances, how do I write my code to suffer fools lightly, and still be durable and extendable? So far, I'm quite happy with the short, direct try:except block acting as a guardian.

Don't give into that temptation.

Suppose that the user inputs an invalid number say "45a", and then the default is taken. What happens? In the best case its obvious the user that something went wrong. However, its probably not obvious exactly what went wrong. However, your program knows exactly what went wrong, but chose not to tell the user. You've just made things harder for the user. Even worse, what if the user doesn't notice that something is wrong. The user assumes that his input was read correctly and takes the program's output as accurate and acts on it. Now they have incorrect information, and don't know it!

As far as I'm concerned nothing good can happen from ignoring invalid user input. Tell the user they goofed.

EDIT

Firstly, there is a very large important difference between tinyurl and your function. Tinyurl never overrides the user input with defaults. It doesn't try and guess what you really meant when you give it nonsense. Your function overrides user input and tries to guess what the user really wanted. Its the guessing and overriding that is a really bad idea. The only effect of doing those is to mask bugs.

Secondly, while tinyurl can get away with it as a website, I don't think "suffering fools lightly" is a good idea. We all play the fool occasionally, and when that happens I want my programs to slap me in the face, not play along. When I introduce a bug in my program, I want that bug to manifest itself as close as possible to point where I introduced it. That way I should have the easiest time tracing the problem. What your method does is prevent function from dying because I passed it the wrong types, but then exhibits incorrect behavior later on. As far as I can tell, there are no circumstances in which that will be helpful to me.


Parsing glider data from IGC files I have to do a program in PHP that reads IGC files like this IGC file and gets records about the glider.
For now I came up with something like this:
<?php

        class Pilot {
            public $name;
            public $gliderType;
            public $competitionId;
            public $gpsDatuml;
            public $competitionClass;
            public $startPoint;
            public $endPoint;
        }
        $pilot = new Pilot();
        $myFile = new SplFileObject("https://xcportal.pl/sites/default/files/tracks/2020-06-09/069daro396091568.igc", 'r', 10);
        for($i=0; $i < 10; $i++) {
            $information =  $myFile->fgets();
            if(($pos = strpos($information, "PILOT")) !== FALSE){
                $pilot->name = substr($information, $pos+7);
            }
            if(($pos = strpos($information, "GLIDERTYPE")) !== FALSE){
                $pilot->gliderType = substr($information, $pos+12);
            }
            if(($pos = strpos($information, "COMPETITIONID")) !== FALSE){
                $pilot->competitionId = substr($information, $pos+15);
            }
            if(($pos = strpos($information, "GPSDATUM")) !== FALSE){
                $pilot->gpsDatuml = substr($information, $pos+10);
            }
            if(($pos = strpos($information, "CLASS")) !== FALSE){
                $pilot->competitionClass = substr($information, $pos+7);
            }
            // echo strpbrk($information, "PILOT");
            // echo substr($information, 10);
        }
        echo $pilot->name;
        echo $pilot->gliderType;
        echo $pilot->competitionId;
        echo $pilot->gpsDatuml;
        echo $pilot->competitionClass;

        $myFile = null;
    ?>

but I wonder if there is anyway to do it better that I haven't figured out already.
Also is it good practice to keep properties of the class as a private field or can I leave them as public if any other class doesn't use that property?

I don't think I understand the point of declaring a class if you aren't going to write any methods in it. I mean why go the expense of declaring a class just to store some variables outside of the global scope.  Assuming this class can never populate its properties without parsing a document, so it make sense to have its constructor initiate the file reading and generate values for the properties.

I haven't personally used SPLFileObject before, but the manual says that:

the 2nd parameter $open_mode defaults to r and
the 3rd parameter $use_include_path expects a boolean value.  I expect that that 10 is not doing what you intend.

Reading a limited number of lines from the file with fgets() is a professional decision that shows real care and deliberate scripting.  Removing all debate regarding micro-optimization or if reading the whole file is an "affordable" cost, I love that you are endeavoring to ONLY "read what you need".

By my count, there are 3 factors which should halt the loop of reading of lines:

there are no more lines in the file
a line is encountered that does not contain a colon
you have extracted all relevant details from the file up to your self-imposed "magic counter" 10

The first two logical break points should be baked into the loop's breaking syntax.  The third is probably something that you don't want to get into the habit of.  Another developer may look at the code (without seeing a sample input file) and think: "Why is this operation limited to 10? Why not 20?  Why not 5?"  It is best to avoid magic numbers, or if you need to have them, write a comment explaining why it is implemented.

That battery of if conditions is considerably less wise.  Think about it.  If the first if is satisfied -- why would you want to bother executing the other four checks?  They should never be satisfied if the first one is.  While I am going to suggest a different technique in my snippet to follow, your snippet at the very least would be improved by using subsequent elseif conditions.

Because I'm rather familiar with regex, I find using regex with a lookup array to be a very convenient technique to extract your targeted values from the remote file.  Not only does regex afford a more compact script, it maintains the flexibility of your original script by allowing the targeted lines to exist in any order.
A code suggestion:
class Pilot
{
    public $name;
    public $gliderType;
    public $competitionId;
    public $gpsDatuml;
    public $competitionClass;
    public $startPoint;  // I don't know where/how this gets populated
    public $endPoint;  // I don't know where/how this gets populated

    private $keywords = [
        'PILOT' => 'name',
        'GLIDERTYPE' => 'gliderType',
        'COMPETITIONID' => 'competitionId',
        'GPSDATUM' => 'gpsDatuml',
        'CLASS' => 'competitionClass',
    ];

    public function __construct(string $url)
    {
        $this->populatePropertiesFromUrl($url);
    }

    public function populatePropertiesFromUrl(string $url): void
    {
        $pattern = '~(' . implode('|', array_keys($this->keywords)) . '): (.*)~';

        $file = new SplFileObject($url);
        while (!$file->eof()
            && ($line = $file->fgets())
            && strpos($line, ':') !== false
        ) {
            if (preg_match($pattern, $line, $match)) {
                $this->{$this->keywords[$match[1]]} = $match[2];
            }
        }
    }
}
$pilot = new Pilot('https://xcportal.pl/sites/default/files/tracks/2020-06-09/069daro396091568.igc');
var_export(get_object_vars($pilot));

Output (should be):
array (
  'name' => 'DARIUSZ KISZKO',
  'gliderType' => 'ADVANCE IOTA 2',
  'competitionId' => '0000',
  'gpsDatuml' => 'WGS-84',
  'competitionClass' => 'Paraglider (Standard)',
  'startPoint' => NULL,
  'endPoint' => NULL,
)

*Disclaimer: I am making some assumptions about the text in the remote file.  I don't actually know how the text may vary.  I don't know if my suggested code will hold up against other files.  You may wish to adjust my loop breaking algorithm.


Is this file I/O utility efficient or is there a lot of fat? This is a class/method that manages files being written to and read from. I have a lot of code, and it grew as I tried to get it to work properly. Is there a better way to write this (i.e. trim the fat) or is it written properly?

import java.io.*;

public class DataBase {
public static String getExtension(File f) {
    String ext = null;
    String s = f.getName();
    int i = s.lastIndexOf('.');

    if (i > 0 && i < s.length() - 1) {
        ext = s.substring(i + 1).toLowerCase();
    }
    return ext;
}

public static String readFile(String file) {
    String data = "";
    if (!new java.io.File(file).exists()) {
        return data;
    }
    File f = new File(file);
    FileInputStream fStream = null;
    BufferedInputStream bStream = null;
    BufferedReader bReader = null;
    StringBuffer buff = new StringBuffer();

    try {
        fStream = new FileInputStream(f);
        bStream = new BufferedInputStream(fStream);
        bReader = new BufferedReader(new InputStreamReader(bStream));
        String line = null;

        while ((line = bReader.readLine())!=null) {
            //line = bReader.readLine();

            if (line.length() > 0) {
                if (line.contains("
")) {
                    line = line.replaceAll("
", " ");
                    String tempLine = "";
                    while ((tempLine.trim().length() < 1)
                            && bStream.available() != 0) {
                        tempLine = bReader.readLine();
                    }
                    line = line + tempLine;
                }
                buff.append(line += "\n");

            }
        }

        fStream.close();
        bStream.close();
        bReader.close();

    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
    return buff.toString();

}

public static boolean writeToFile(String fileName, String data,
        boolean append) {
    boolean isWrite = false;
    int dirIndex = fileName.lastIndexOf(getPathSeparator());
    if (dirIndex != -1) {
        String dir = fileName.substring(0, dirIndex) + getPathSeparator();
        java.io.File fDir = new java.io.File(dir);
        if (!fDir.exists()) {
            if (!fDir.mkdirs()) {
                return false;
            }
        }
    }
    try {
        java.io.FileOutputStream fout = new java.io.FileOutputStream(
                fileName, append);
        java.nio.channels.FileChannel fChannelWriter = fout.getChannel();
        byte[] bytesToWrite = data.getBytes();
        java.nio.ByteBuffer bBuffW = java.nio.ByteBuffer.wrap(bytesToWrite);
        fChannelWriter.write(bBuffW);
        fChannelWriter.close();
        fout.close();
        isWrite = true;
    } catch (java.io.IOException ex) {
        System.out.println("Error Occured: Unable to write to file ("
                + fileName + ")");
        isWrite = false;
    }
    return isWrite;
}

public static String getPathSeparator() {
    return java.io.File.separator;
}
}

DataBase is a poor name for a collection of file I/O convenience routines.  Especially one whose readFile() function swallows empty lines and strips
 tags as it reads.  What exactly are you trying to accomplish with this class?  I suspect that you have decomposed your problem poorly, and this class shouldn't exist.

The getExtension() function could be more succinct.

public static String getExtension(File f) {
    String s = f.getName();
    int i = s.lastIndexOf('.');

    return (i > 1 && i < s.length() - 1) ?
        s.substring(i + 1).toLowerCase() :
        null;
}

I've changed the condition from i > 0 to i > 1 since the names of Unix dotfiles (e.g. .profile) are not usually thought of as file extensions.  That's a judgement call for you to make.

I think your readFile() should take a File rather than a String parameter.  If you design your API so that you consistently use File to represent file paths and String to represent file content data, then it will be harder to accidentally call your function with swapped arguments (especially for writeToFile()).  Also, if you call the other function writeToFile(), then you should rename readFile() to readFromFile() for consistency.

Swallowing exceptions is usually a bad idea, even if you print the stack trace.  I would just let any IOExceptions propagate.

You're just using the default character encoding, so you don't need to bother with constructing InputStreams — just use Readers.

Testing for String.contains() before doing String.replaceAll() just repeats the work needlessly.  String.replaceAll() will just return the original string anyway if no replacement needs to be performed.

I don't know what you are trying to accomplish with the tempLine stuff.  Since you initialize tempLine = "", it will always be true that tempLine.trim().length() is 0, so all of that is dead code.

Assuming you don't need thread safety, just use StringBuilder instead of StringBuffer.

Doing a string concatenation in buff.append(line + "\n") defeats the efficiency gains of using a StringBuffer.

All together…

public static String readFromFile(File f) throws IOException {
    if (!f.exists()) {
        // I find this behavior surprising; I would have expected a
        // FileNotFoundException to be thrown instead.
        return "";
    }

    FileReader r = new FileReader(f);
    BufferedReader br = null;
    try {
        br = new BufferedReader(r);
        StringBuilder buff = new StringBuilder();

        String line;
        while (null != (line = br.readLine())) {
            if (line.length() > 0) {
                line = line.replaceAll("
", " ");
                buff.append(line).append('\n');
            }
        }
        return buff.toString();
    } finally {
        if (br != null) br.close();
        r.close();
    }
}

The code could be a bit less clumsy if you use Java 7's try-with blocks.

writeToFile() can be simplified by a lot.  For simplicity, I would just use a FileWriter instead of the java.nio API.

public static void writeToFile(File f, String data, boolean append) throws IOException {

    File parent = f.getParentFile();
    if (!parent.exists()) {
        parent.mkdirs();
    }

    FileWriter w = new FileWriter(f, append);
    try {
        w.write(data);
    } finally {
        w.close();
    }
}

With the simplification to writeToFile(), getPathSeparator() might not be necessary anymore.


Concurrent hero vs monster program I have made a simple concurrent program. It represents a battle between a hero and a monster. I am beginner in concurrency so I just want to hear your opinion. What are bad and good sides of my code and how can I improve it?

Interface Attack

public interface Attack {
public void attack(Object s, int dmg);
}

Main class

public class Main {
public static void main(String[] args) {
    Monster monster = new Monster(50, "monster");
    Hero hero = new Hero (50, "hero");
    ThreadFight tf = new ThreadFight(hero, monster);

    Thread t1 = new Thread (new Runnable() {
        @Override
        public void run() {
            tf.hitHero();
        }
    });

    Thread t2 = new Thread (new Runnable() {
        @Override
        public void run() {
            tf.hitMonster();
        }
    });

    t1.start();
    t2.start();
}
}

Class Hero

public class Hero implements Attack{
private int hp;
private String name;
private Monster monster;

public Hero (int hp, String name) {
    this.hp = hp;
    this.name = name;
}

public int getHp() {
    return this.hp;
}

public String getName() {
    return this.name;
}

public void decreaseHp(int dmg) {
    this.hp = getHp() - dmg;
}

public void setMonster(Monster monster) {
    this.monster = monster;
}

@Override
public void attack(Object s, int dmg) {
    if (s instanceof Monster) {
        Monster m = (Monster) s;
        System.out.println("Monster hp: " + m.getHp());
        System.out.println("Hero dealt " + dmg + " damage");
        m.decreaseHp(dmg);
        System.out.println("Monster hp is now: " + m.getHp());
        System.out.println("--------------------------");
    }
}
}

Class Monster

public class Monster implements Attack{
private int hp;
private String name;
Hero hero;

public Monster (int hp, String name) {
    this.hp = hp;
    this.name = name;
}

public int getHp() {
    return this.hp;
}

public String getName() {
    return this.name;
}

public void decreaseHp(int dmg) {
    this.hp = getHp() - dmg;
}

public void setHero(Hero hero) {
    this.hero = hero;
}

@Override
public void attack(Object s, int dmg) {
    if (s instanceof Hero) {
        Hero h = (Hero) s;
        System.out.println("Hero hp: " + h.getHp());
        System.out.println("monster dealt " + dmg + " damage");
        h.decreaseHp(dmg);
        System.out.println("Hero hp is now: " + h.getHp());
        System.out.println("--------------------------");
    }
}
}

Class ThreadFight

public class ThreadFight{
Hero hero;
Monster monster;
Random random = new Random();
Object lock = new Object();

public ThreadFight(Hero hero, Monster monster) {
    this.hero = hero;
    this.monster = monster;
}

public void hitMonster() {

    while (monster.getHp() > 0 && hero.getHp() > 0) {
        synchronized (lock) {
            try {
                Thread.sleep(2000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            hero.attack(monster, random.nextInt(20));

        }
        if (monster.getHp() < 0) {
            System.out.println("Hero won.");
        }
    }
}

public void hitHero() {

    while (hero.getHp() > 0 && monster.getHp() > 0) {
        synchronized (lock) {
            try {
                Thread.sleep(2000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            monster.attack(hero, random.nextInt(20));

        }
        if (hero.getHp() < 0) {
            System.out.println("Monster won.");
        }
    }
}
}

One major problem with this code that I can see is that you need to consider what you are synchronizing, and why. I understand your main objective here is learning concurrency but I've also added some other suggestions you may or may not want to use.

Concurrency

There are a couple of issues with the way you are synchronizing things. In your case, the things you'd want to prevent happening are:

Interleaving on the players' attacks. This might happen if two
separate threads try and damage the same enemy.
Preventing both the Hero and Monster 'winning' (killing each other). Note: you might not want to prevent this

The synchronized blocks will prevent any code with the same lock executing at the same time. Therefore you need to synchronize:

Modifications to health of self or enemy.
Checks of health of self or enemy.

You should not synchronize the Thread.sleep(); calls because each thread will have stop and wait for all the other ones to stop sleeping. So what does this look like for your code? Let's see:

public void hitHero() {
    boolean isFighting = true;
    while (isFighting) {
        try {
            Thread.sleep(2000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        synchronized (lock) {
            monster.attack(hero, random.nextInt(20));
            if (hero.getHp() < 0) System.out.println("Monster won.");
            isFighting = hero.getHp() > 0 && monster.getHp() > 0;
        }

    }
}

Note that I have used the boolean isFighting in order to move the checks to HP inside the synchronized block.

This solution will not prevent the two from killing each other if one dies while the other is waiting to attack. To prevent this, you'll need to add other checks inside the synchronized block.

Encapsulation

Consider the Monster and the Hero. They both share some common traits. They both have HP, a name, and can both attack something. Why not make a parent class (let's call it Fighter) that is not just an interface, but provides implementation for attack, and a hp + name:

public class Fighter {
    private int hp;
    private String name;

    public Fighter (int hp, String name) { ... }
    public int getHp() { ... }
    public String getName() { ... }
    public void decreaseHp(int dmg) { ... }

    public void attack(Fighter f, int dmg) {
        System.out.println(f.name + " hp: " + f.getHp());
        System.out.println(name + " dealt " + dmg + " damage");
        f.decreaseHp(dmg);
        System.out.println(f.name + " hp is now: " + f.getHp());
        System.out.println("--------------------------");
    }

The great thing about this is that you lose the need for the instanceof check, since you know you can attack any fighter you want. Now adding a new type of fighter is super easy:

public class Hero extends Fighter {
    public Hero (int hp, String name) {
        super(hp, name);
    }
}

In addition to this, I would certainly consider making all the variables inside ThreadFight private, and creating + starting the two Threads inside ThreadFight's constructor (or inside a startFight() method).

Overall though, I think you've done a good job! You're running the threads correctly and nearly got the synchronization right. It can take a while to fully understand what's happening with multiple threads and locks. Though there are possible improvements the Monster and Hero were also built well. It's a nice little program and you could certainly expand upon it if you wanted to practice more complex threading situations.


Hex boardgame with fast random rollouts (OpenAI gym framework) I implemented the boardgame Hex using the OpenAI gym framework with the aim of building a bot/AI player that can learn through self-play and expert iteration (details Note: not my paper; I am merely reproducing it).
The initial agent uses Monte-Carlo tree search (MCTS), and I will compare myself against it to evaluate the strength of different bots. MCTS involves simulating the game with random moves (called a rollout) and this is done A LOT (>1,000 games played per move in the actual game), so this rollout speed matters to me. Indeed, when I profile my code, the bottleneck is said rollout, and, more specifically, the test if the game has ended.
Currently, I check if the game is finished using the following mechanism (I'm sure there is a name for it, but I don't know it):

Pad the board with 1 extra row/column and place stones on the west/east side (player white/blue) or north/south side (player black/red) (cached at the start of the game)
Find all the connected regions for the current player (cached from previous turn)
Place stone on board
check neighborhood of stone and (a) start new region if unconnected, (b) add to the region with lowest region index
if multiple regions are in the neighborhood, merge them with the region that has the lowest index

I assign index 1 to the stones in the north/west (black/white) padding, and can then efficiently test if the game is over by checking the south-east corner. If it has region index 1, it is connected to the opposite side and the game has finished.
The full code of the game is available on GitHub together with a MWE that performs a random rollout. It's not a big repo (maybe 500 lines). The critical function is this one
    def flood_fill(self, position):
        regions = self.regions[self.active_player]

        current_position = (position[0] + 1, position[1] + 1)
        low_x = current_position[1] - 1
        high_x = current_position[1] + 2
        low_y = current_position[0] - 1
        high_y = current_position[0] + 2
        neighbourhood = regions[low_y:high_y, low_x:high_x].copy()
        neighbourhood[0, 0] = 0
        neighbourhood[2, 2] = 0
        adjacent_regions = sorted(set(neighbourhood.flatten().tolist()))
        adjacent_regions.pop(0)

        if len(adjacent_regions) == 0:
            regions[tuple(current_position)] = self.region_counter[self.active_player]
            self.region_counter[self.active_player] += 1
        else:
            new_region_label = adjacent_regions.pop(0)
            regions[tuple(current_position)] = new_region_label
            for label in adjacent_regions:
                regions[regions == label] = new_region_label

with the most expensive line being adjacent_regions = sorted(set(neighbourhood.flatten().tolist())). I'm wondering if this can be implemented in a nicer way, either by using a different algorithm or vectorizing the code more, more intelligent caching, ...
Of course, I'm also happy with any other comment on the code.
Disclaimer: I found a basic hex implementation in an old commit in the OpenAI gym repo, which I used as a base to work off. Most of the code has changed, but some of it (e.g., the render function) I did not write myself.

When reading this function alone, without any surrounding code, I wonder where the initial + 1 for the position comes from. That looks like an off-by-one bug to me. I don't know whether it is indeed a bug, it's just suspicious.
The calls to tuple() look redundant since the current_position already is a tuple. Doesn't your IDE warn about such things?
The word position is a bad name since it is ambiguous. It could either mean an (x, y) tuple or the complete (board, player_to_move) tuple, like in the sentence "in this position, Red should resign". A better name would be last_move or prev_move.
Is there a good reason why you use a tuple at all? Having two variables x and y would make the code pretty clear. These variable names are short enough that you don't need the low_x and related variables anymore.
Do you need the call to tolist() at all?
Instead of generating a 2-dimensional matrix, it could be more efficient if you just took the 6 neighbor regions explicitly and individually. That way you also get rid of the pop(0). I don't know whether that is faster in Python though.


Exporting doc types using queues and multithreading For a while I have been interested in seeing if some tasks work well when split across multiple threads.

On the one project I have been busy with I have been creating a lot of small utility apps, to do single tasks for me, so this morning while modifying one I thought I would give it a try.

What I wrote seems to work, although I am not sure if it is the best way, or recommended way to do something like this, so I want to ask what is wrong with my code and how could it be done better. What it is doing is not really important, and I am not interested if this sort of task should be split up, but more along the lines of how good or bad is the code that I have written.

private static void ExportDocTypes(IEnumerable<string> docTypes)
{
    var queue = new Queue<string>(docTypes);
    var tasks = new List<Task>();
    for (byte i = 0; i < threadCount; i++)  //threadCount is a const set to 4.
    {
        Action a = () =>
        {
            while(queue.Count() > 0) {
                var type = queue.Dequeue();
                ExportDocuments(type, i);  //i is only sent for response.write
            }
        };
        var t = new Task(a);
        t.Start();
        tasks.Add(t);
        System.Threading.Thread.Sleep(10);
    }
    while (tasks.Count() > 0)
    {
        foreach (var t in tasks)
        {
            if (t.IsCompleted)
            {
                t.Dispose();
                tasks.Remove(t);
            }
        }
        System.Threading.Thread.Sleep(1000);
    }
}

Rather than reviewing your specific code -- which at first glance seems very, very buggy -- let me answer your question directly:

  What are the best practices with multithreading in C#?

Don't. Multithreading is a bad idea. Programs are hard enough to understand already with a single point of control flow in them; why on earth would you want to add a second, third, fourth... ? Do you enjoy tracking down incredibly difficult-to-reproduce, impossible-to-understand bugs? Do you have an encyclopaedic knowledge of the differences between weak and strong memory models? No?  Then don't write multithreaded programs!  Processes are a perfectly good unit of work; if you have work to do, spawn a process.
Oh, so you want to do multithreaded programming anyways.  In that case use the highest level of abstraction available.  Programming against threads directly means writing programs about workers when you could be writing programs about jobs. Remember, workers are just means to an end; what you really want to get done are the jobs.  Use the Task Parallel Library: tasks are jobs, threads are workers. Let the TPL figure out for you how to make efficient use of workers.
Wait, let's go back to point one again.  Ask yourself do you really need concurrency? With true concurrency we have two or more points of control in a program actually active at the same time. With simulated concurrency the two points of control take turns using the processor, so they are not actually concurrent. Most of the time we end up with simulated concurrency, because there are more threads than there are processors. This leads us naturally to the conclusion that perhaps actual concurrency is not what we need. Much of the time what people really want is asynchrony; concurrency is one way to achieve asynchrony, but it is not the only way.  Use the new async/await functionality in C# 5 to represent asynchronous workflows.
If you are confused by the previous point, and think wait a minute, there is no way to achieve asynchrony without multiple threads of control in a process, read Stephen Cleary's article There Is No Thread.  Don't try to write multithreaded programs until you understand it.
At all costs avoid shared memory. Most threading bugs are caused by a failure to understand real-world shared memory semantics. If you must make threads, treat them as though they were processes: give them everything they need to do their work and let them work without modifying the memory associated with any other thread. Just like a process doesn't get to modify the memory of any other process.
Oh, so you want to share memory across threads even though doing so is incredibly bug prone? Again, let me go back to use the highest level tool available to you. Those tools were written by experts. You're not an expert. Do you know how to make a lazily-computed value written to a field such that you are absolutely guaranteed that the lazy computation executes no more than once and the field is never observed to be in an inconsistent state no matter what is in any given processor cache?  I don't. You probably don't. Joe Duffy does, which is why you should use the Lazy<T> primitive he wrote rather than trying to write your own.
Oh, so you want to write your own code that shares memory? Then take the lock for heaven's sake. I see so much buggy code where people get into their heads that locks are expensive and they write this convoluted godawful wrong code that evades taking a lock that would have cost them twelve nanoseconds to just take. Avoiding a lock to save on its cost is like complaining that the electricity takes too long to get to the light bulb when you flip the switch. Locks are a powerful tool for ensuring memory consistency; if you're going to program shared memory at the thread level, you avoid locks at your peril.
If you use Thread.Sleep with an argument other than zero or one in any production code, you are possibly doing something wrong. Threads are expensive; you don't pay a worker to sleep, so don't pay a thread to sleep either. If you are using sleeps to solve a correctness issue by avoiding a timing problem -- as you appear to be in your code -- then you definitely have done something deeply wrong. Multithreaded code needs to be correct irrespective of accidents of timing.


Is this implementation of signed URLs reasonably secure? I'm trying to implement signed URLs for short lived access to static files.
The idea is:

generate an URL with an expiration timestamp (e.g. https://example.com/file.png?download=false&expires=1586852158)
sign it with HMACSHA256 and a private key and append the signature at the end of URL (e.g. https://example.com/file.png?download=false&expires=1586852158&signature=6635ea14baeeaaffe71333cf6c7fa1f0af9f6cd1a17abb4e75ca275dec5906d1

When i receive the request on the server, I take out the signature parameter and verify that the rest of the URL signed with HMACSHA256 and the same private key results in the same signature.

The implementation is as follows:

    public static class URLSigner
    {
        private static string GetSignatureForUri(string uri, byte[] key)
        {
            var hmac = new HMACSHA256(key);
            var signature = hmac.ComputeHash(Encoding.UTF8.GetBytes(uri));
            var hexSignature = BitConverter.ToString(signature).Replace("-", string.Empty).ToLowerInvariant();
            return hexSignature;
        }

        public static string SignUri(string uri, byte[] key)
        {
            var hexSignature = GetSignatureForUri(uri, key);
            return QueryHelpers.AddQueryString(uri, new Dictionary<string, string> { { "signature", hexSignature }});
        }

        public static bool VerifyUri(string uri, byte[] key)
        {
            var signatureRegex = "[\\?&]signature=([a-z0-9]+)$";
            var signatureMatch = Regex.Match(uri, signatureRegex);
            if (!signatureMatch.Success || signatureMatch.Groups.Count != 2)
                return false;
            var parsedSignature = signatureMatch.Groups[1].Value;

            var originalUri = Regex.Replace(uri, signatureRegex, "");
            var hexSignature = GetSignatureForUri(originalUri, key);
            return hexSignature == parsedSignature;
        }
    }

and it's used like so:

var signedUri = URLSigner.SignUri("https://example.com/file.png?download=false", secretKey);
var isVerified = URLSigner.VerifyUri(signedUri, secretKey);

The timestamp is verified separately on the request handler.

Is this implementation of signed URLs reasonably secure?

It looks like a rather straightforward use of HMAC. So I would presume it is safe from that perspective.

A note about terminology. Personally I would talk about an authentication tag or simply HMAC value because people often associate signatures with asymmetric algorithms such as RSA. The same goes for "private key"; I'd use secret key for symmetric keys as in your code.

Some remarks:

the regular expression is used twice, it seems more logical to match the entire GET request and make sure it ends with the group with the signature in it (even if that just means using .*);
to remove one way of attacks, I'd make sure that there aren't two authentication tags in the GET request;
to remove another, I'd include a sequence number or time stamp so that replay attacks are not possible;
hexadecimals are not very efficient, I'd use a base64url encoder, it's made for it;
the groups count check is unnecessary, either the regex matches or it doesn't.

One idea is to have a method split the URI and the signature, you don't need the authentication tag after verification after all - you could for instance use a tuple with the verification status and the URL without signature (as optional value), or use a yucky output parameter. Check your coding standards before doing either of these though.


Dice roll simulator (updated) created using Python 3.5 and PyQt5 from random import choice
from sys import (exit, argv)
from time import sleep

from PyQt5.QtCore import Qt
from PyQt5.QtGui import (QIcon, QPixmap, QFont)
from PyQt5.QtWidgets import (QToolTip, QPushButton, QApplication,
                             QWidget, QLabel, QComboBox, QDesktopWidget)

ICON = 'icon.png'
DICE = ('dice_1.png', 'dice_2.png', 'dice_3.png', 'dice_4.png', 'dice_5',
        'dice_6.png')
ROLLING_ANIMATION = 'dice_rolling_animation.png'
SCALED_PARMS = 162, 302, Qt.KeepAspectRatio, Qt.FastTransformation
ONE_DIE_PARMS = 1427, 30, 162, 201
TWO_DICE_PARMS = 1265, 30, 324, 201
SLEEP_TIME = 0.5

class DiceRollSimulator(QWidget):

    def __init__(self):
        super().__init__()

        self.initUI()

    def initUI(self):
        self.die1 = QLabel(self)
        self.die1.setPixmap(QPixmap(choice(DICE)).scaled(*SCALED_PARMS))
        self.die1.move(0.5, 0.5)

        self.die2 = QLabel(self)
        self.die2.setPixmap(QPixmap(choice(DICE)).scaled(*SCALED_PARMS))
        self.die2.move(162, 0.5)
        self.die2.setVisible(False)

        self.btn = QPushButton('Roll', self)
        self.btn.setFont(QFont('SansSerif', 20))
        self.btn.setToolTip('Click to Roll Die')
        self.btn.clicked.connect(self.rolldie)
        self.btn.resize(166, 43)
        self.btn.move(-2, 161)

        self.dice_amount = QComboBox(self)
        self.dice_amount.addItems(['1', '2'])
        self.dice_amount.activated[str].connect(self.dice_amount_changed)
        self.dice_amount.move(135, -2)

        QToolTip.setFont(QFont('SansSerif', 10))
        QApplication.desktop()
        screen = QDesktopWidget().screenGeometry()
        self.setFixedSize(162, 201)
        x = screen.width() - self.width() - 15
        self.move(x, 1)
        self.setWindowTitle('Dice Roll Simulator')
        self.setWindowIcon(QIcon(ICON))
        self.show()

    def rolldie(self):
        self.die1.setPixmap(QPixmap(ROLLING_ANIMATION).scaled(*SCALED_PARMS))
        self.die2.setPixmap(QPixmap(ROLLING_ANIMATION).scaled(*SCALED_PARMS))
        QApplication.processEvents()
        sleep(SLEEP_TIME)
        self.die1.setPixmap(QPixmap(choice(DICE)).scaled(*SCALED_PARMS))
        self.die2.setPixmap(QPixmap(choice(DICE)).scaled(*SCALED_PARMS))
        QApplication.processEvents()

    def dice_amount_changed(self):
        geo = str(self.geometry())
        geo2 = geo[19:len(geo) - 1]
        loc = [int(x) for x in geo2.split(',')]

        if self.dice_amount.currentText() == '1':
            self.setFixedSize(162, 201)
            x = loc[0] + 154
            y = loc[1] - 30
            self.move(x, y)
            self.die2.setVisible(False)
            self.btn.resize(166, 43)
            self.dice_amount.move(132, -2)

        else:
            self.setFixedSize(324, 201)
            x = loc[0] - 170
            y = loc[1] - 30
            self.move(x, y)
            self.die2.setVisible(True)
            self.btn.resize(328, 43)
            self.dice_amount.move(294, -2)

if __name__ == '__main__':

    app = QApplication(argv)
    ex = DiceRollSimulator()
    ex.show()
    exit(app.exec_())

This is the updated version of the dice roll simulator code I posted some time ago. I've made several changes and additions including ones suggested by users here.

Nice code, so only some comments.

ONE_DIE_PARMS = 1427, 30, 162, 201
TWO_DICE_PARMS = 1265, 30, 324, 201

Never used, typo in the first of them.

In contrast, there are other magic numbers in your code which deserve names for them.

DICE = ('dice_1.png', 'dice_2.png', 'dice_3.png', 'dice_4.png', 'dice_5',
        'dice_6.png')

may become

DICE_TEMPLATE = 'dice_{}.png'
DICE = [DICE_TEMPLATE.format(i) for i in range(1, 7)]

(BTW. it's common to use plural for names of collections - DICES)

geo2 = geo[19:len(geo) - 1]

may become

geo2 = geo[19:-1]

as negative numbers in slices mean "from the end".


JavaScript code to add buttons and slowly remove them My javascript code is working, however i am seeking suggestions on how I can go about improving my existing code for reusability and testability. The code is pretty straight forward, I have three buttons "add", "slow remove" and "clear". The add button adds 40 buttons to the DOM dynamically with id + index. The slow remove button, uses setTimeout and CSS transition opacity to slowly fade out the 40 buttons, and the clear button clears the 40 buttons immediately.

Buttons JS:

  (function () {

    /* button_add when clicked dynamically generates 40 buttons with indexes
       when clicked an alert is display with button's id */
    const CLS = ["thin", "button"];
    const BUTTON_NUM = 40;
    let buttons = [];
    let add_btns = document.querySelector('.button_add');
     add_btns.addEventListener("click", function () {
         let i = 0;
        while (i <= BUTTON_NUM) {
            let btn = document.createElement("BUTTON");
            buttons.push(container.appendChild(btn));
            i++;
            for (let button in buttons) {
                btn.setAttribute('id', "button " + button);
                btn.classList.add(...CLS);
                btn.innerHTML = "Button # " + button;
            }
            btn.addEventListener("click", function () {
                let id = this.id;
                alert("my id is: " + id);
            });
        }
     });

    /* button_slow_remove when clicked 40 buttons fade out slowly and buttons array is
       emptied */
    let slow_remove_btns = document.querySelector('.button_slow_remove');
    slow_remove_btns.addEventListener("click", function () {
            buttons.forEach(function (el) {
                el.classList.add('slow-fade');
            });
            buttons.length = 0;
    });

    /* button_clear when clicked 40 buttons are cleared immediately and buttons array is
           emptied */
    let clear_btns = document.querySelector('.button_clear');
    clear_btns.addEventListener("click", function () {
        buttons.forEach(function (el) {
            el.classList.add('clear');
        });
        buttons.length = 0;
    });
}) ();

Buttons.css:

html section button, body section button {
    align-self: center;
    background: transparent;
    padding: 1rem 1rem;
    margin: 0 1rem;
    transition: all .5s ease;
    color: #41403E;
    font-size: 2rem;
    letter-spacing: 1px;
    outline: none;
    box-shadow: 20px 38px 34px -26px rgba(0, 0, 0, 0.2);
    border-radius: 255px 15px 225px 15px/15px 225px 15px 255px;
}

div#container button {
    display: inline-flex;
    align-self: center;
    background: transparent;
    padding: 1rem 1rem;
    margin:  .2rem 1rem;
    transition: all .5s ease;
    color: #41403E;
    font-size: .3rem;
    letter-spacing: 1px;
    outline: none;
    box-shadow: 20px 38px 34px -26px rgba(0, 0, 0, 0.2);
    border-radius: 255px 15px 225px 15px/15px 225px 15px 255px;
}

html section button:hover, div#container button:hover, body section button:hover {
    box-shadow: 2px 8px 4px -6px rgba(0, 0, 0, 0.3);
}

section button.dotted, body section button.dotted.thick {
    border: dotted 5px #41403E;
}

div#container button.thin, body section button.lined.thin {
    border: solid 2px #41403E;
}

.button--main, button.button{
    cursor: pointer;
}

.clear {
    display: none!important;
}

.slow-fade {
    opacity: 0;
    -webkit-transition: opacity 7s linear;
    -moz-transition: opacity 7s linear;
    transition: opacity 7s linear;
}

@media (max-width: 620px) {
    body h1 {
        margin-top: 2rem;
    }
    body section, div#container {
        display: flex;
        flex-direction: column;
        justify-content: center;
        margin-bottom: 1rem;
    }
    body section button {
        align-self: center;
        margin-bottom: 2rem;
    }
}

Naming.
Use a consistent and idiomatic naming style. Most important is consistency within the code base.
JS
For JS use cammelCase for all names appart from object that are instanciated with the new token, in that case use PascalCase. For constants you can use UPPER_CASE_SNAKE but as objects and arrays are often declared as constants you can end up with a lot of UPPER_CASE_SNAKE in the code, which does not read well.
There seams to be no general agreement as to what style to use. I personally only use UPPER_CASE_SNAKE for constants that are basic types (Number, Bool, String, etc..) and for object properties that are not writable.
CSS
Currently your CSS naming is inconsistent. This will make it harder to write good code as you will be spending mental energy remembering what style you used for what name, eg you have the two classes named button_slow_remove and slow-fade you remember the name not the hyphen or underscore, later you come back to the project and you will forever be checking "Was that name underscore or hyphen"
For CSS you can use Block Element Modifier (BEM) naming convention. The link provided a good introduction with plenty of resources to get you started.
Note BEM is but one of many and there is nothing wrong with defining your own, as long as you are consistent.
Plurals
Names give hints to a variables content. We pluralize arrays, lists, sets, etc (sometimes even strings if we are using them as an array of characters), and use the singular for single items.
You are using let add_btns = document.querySelector('.button_add'); and someone, even you coming back later will automatically think buttons (an array / array like), its a buton, use the singular const addBtn = document.querySelector('.button_add');
Also it is ok to use common abbreviations, btn is just as good a name as button
Always BE CONSISTENT!

Code Style

NEVER start a line with a ( unless it is part of a multi line statement or expression. You have (function () { If someone adds a line above this and is one of those crazzy people that say they don't need to terminate lines

Eg
var a = 0
var b = a  // << this line is exterminated

 (function() {  // << will parse as var b = a(function() { and throw an error

So play it safe and add the ; for those that don't.
var a = 0
var b = a  // << this line is un-terminated

;(function() {  // the ; terminates any multi line statement or expressions

// while at you can use the shorter form

;(()=>{
    ...code
})()

Use arrow functions to reduce code noise. Code noise is any code that is not needed.

Always declare variables as const, only use var or let if the variable needs to change. The following variables are all incorrectly defined as let yet don't change buttons, add_btns, btn, id, clear_btns, slow_remove_btns

Take care with the indenting.

Reduce code noise. A measure code quality can be defect density, this value is linked to the coder and their experience level. Your defect density will remain consistent (though time will reduce the value), it means more code, more bugs. The easiest way to reduce the number of bugs in your code is to reduce the code size. You have many unneeded lines of code, many due to style.

You have
btn.addEventListener("click", function () {
    let id = this.id;
    alert("my id is: " + id);
});

// less noise
btn.addEventListener("click", (e) => alert("My id is: " + e.target.id));

// or to keep `this`
btn.addEventListener("click", function() { alert("My id is: " + this.id) });

API use

Don't use setAtribute if its not needed. The general rule is setAtribute is only needed for properties that are not part of the DOM's API and needs to be reflected in the markup.

When setting text for an element use the textContent property, ONLY use innerHTML if you are adding HTML

Use an element's id to select it rather than the class name. Using class name forces you to create CSS rules for elements with the same style.

Rather than add the buttons directly to the DOM, which will force a re-flow and slow things down, use a documentFragment, or create an off page container to work with, then when complete add them to the page in one call / one re-flow.

Bugs
You have some bugs in the code.

Your code comments say 40 buttons yet the code generates 41 buttons.

The code at fault is
const BUTTON_NUM = 40;

// ... some code removed

     let i = 0;
    while (i <= BUTTON_NUM) { // from 0 to 40 are 41 steps

    // Should be
    while (i < BUTTON_NUM) {

You are adding id's too often. Inside the while loop you do a for in loop over the buttons you have already created. This means the first button is assigned the same values 41 times.

Testing
You don't write your code for testing, you test code. You have not presented any testing code so there is nothing to review.

Some other points

Keep things simple. You are storing an array of buttons, you are adding events to each fade button. There is no need. Add a single event to the button container, use the event.target argument to get the element clicked on. Don't store elements if you don't need them

Magic numbers and strings are bad. You code has many strings referring to class names all over the place. If you need to make changes, it requires you to make changes in many places, which is a pain and potencial defect.
You started well with const CLS = ["thin", "button"] and  const BUTTON_NUM = 40 but then started mixing in string as needed. Keep all magic values (constants that are subject to design change) in one place, that need only one change to the source.

From the Comments

Try to make it class – zb

NO Don't! JS does not have classes, only objects. class is a syntax used to create objects.
You don't need to create an object, you have correctly encapsulated your code with a singleton, it needs no exposed API (object with properties and functions), and works as a static single instance (It does not need many copies).
OO coding is a complex subject, and most low level coders do not have a good grasp of when and how to use it effectively. Making it a class does not mean it makes it better code.
Rewrite
From the ground up and using an object btns to hold button related consts, and functionality.
To improve portability, or really just make it more flexible I have create functions to cover the common tasks you code was doing. Eg query, addEvent, clickEvent...
I have also deliberately referenced element buttonContainer to show that it is a legitimate way to access elements. But do note that that name is global and not isolated to the singleton.
The layout and CSS have been improvised as you only provided CSS and that had unrelated content.
The code adds 3 events for each of the main ui buttons. The add button adds the buttons to a fragment and then to the page (clearing the previous content beforehand)
Then a final event is added to the button container to manage the alert showing button id.
It is designed to simplify the adding of Remove FX. You need only change 3 lines of JS and that is only data to add a new fade FX. (of course you need to add the HTML and CSS)

;(() => {
    const btns = {
        count : 13,
        className : "buttons buttons--style-default buttons--action-fader",
        text : "Button #", // the # is replaced with the ID
        id : "button#",    // the # is replaced with the ID
        actions : {add : "#addBtn", slowFade : "#slowFadeBtn",clear : "#clearBtn" },
        anim : { slowFade : "buttons--anim-fade", clear : "buttons--anim-clear" },
        animNames : ["slowFade", "clear"],
        reset() {
           btnContainer.innerHTML = "";
           for (const name of btns.animNames) { btnContainer.classList.remove(btns.anim[name]) }
        },
        remove(method) { btnContainer.classList.add(method) },
    };

    const query = query => document.querySelector(query);
    const addEvent = (el, func, type = "click") => el.addEventListener(type, func);
    const clickEvent = event => alert("My id is : " + event.target.id);
    const actionEvent = name => addEvent(query(btns.actions[name]), () => btns.remove(btns.anim[name]));
    const frag = document.createDocumentFragment();

    addEvent(query(btns.actions.add), () => {
        btns.reset();
        for (let i = 0; i < btns.count; i ++) {
            frag.appendChild( Object.assign( document.createElement("button"), {
                    id : btns.text.replace("#", i),
                    className : btns.className,
                    textContent : btns.text.replace("#", i),
                })
            );
        }
        btnContainer.appendChild(frag);
    });
    addEvent(btnContainer, clickEvent);
    btns.animNames.forEach(actionEvent);
}) ();
.buttons {
    background: transparent;
    align-self: center;
}
.buttons--style-default {
    padding: 0.5rem 0.5rem;
    transition: all .5s ease;
    outline: none;
    color: #41403E;
    cursor: pointer;
    font-size: 1rem;
    box-shadow: 20px 38px 34px -26px rgba(0, 0, 0, 0.2);
    border-radius: 255px 15px 225px 15px/15px 225px 15px 255px;
}
.buttons--action-ui {
    align-self: center;
    margin: 0 0.2rem;
}
.buttons--action-fader {
    display: inline-flex;
    margin:  .2rem 0.2rem;
    border: solid 2px #41403E;
}
.buttons--style-default:hover {
    box-shadow: 2px 8px 4px -6px rgba(0, 0, 0, 0.3);
}
/* Animations or animation like rules for buttons */
.buttons--anim-clear {
    display: none!important;
}
.buttons--anim-fade {
    opacity: 0;
    transition: opacity 4s linear;
}

    <button id="addBtn" class="buttons buttons--style-default buttons--action-ui" >Add buttons</button>
    <button id="clearBtn" class="buttons buttons--style-default buttons--action-ui" >Remove buttons</button>
    <button id="slowFadeBtn" class="buttons buttons--style-default buttons--action-ui" >Remove with fade</button>


De Casteljau's algorithm to draw Bezier Curves in OOP and C++ (iterative version) #include <iostream>
#include "Point2d.h"
#include "Collection.h"
#include "Coordinates2d.h"
#include "graphics.h"

#define PRECISION 0.01

Point2d Approximate(double t, Point2d const & pt1, Point2d const & pt2)
{
    double x = pt1.x * (1-t) + pt2.x * t;
    double y = pt1.y * (1-t) + pt2.y * t;

    return Point2d(x, y);
}

Collection<Point2d> GetBezierPoints(Collection<Point2d> const & points)
{
    Collection<Point2d> bezierPoints;

    for(double t = 0 ; t < 1 ; t = t + PRECISION)
    {
        Collection<Point2d> temp1 = points;

        while(temp1.size()>1)
        {
            Collection<Point2d> temp2;

            for(int i = 0 ; i<temp1.size()-1 ; i++)
            {
                Point2d pt1 = temp1[i];
                Point2d pt2 = temp1[i+1];

                temp2.Add(Approximate(t, pt1, pt2));
            }

            temp1 = temp2;
        }

        bezierPoints.Add(temp1[0]);//finally only one point will be left
    }

    return bezierPoints;
}

int main()
{
    Coordinates2d::ShowWindow("De Casteljau's algorithm");

    Collection<Point2d> points;

    points.Add(Point2d(0,0));
    points.Add(Point2d(80,300));
    points.Add(Point2d(120,-150));
    points.Add(Point2d(200,0));

    Collection<Point2d> list = GetBezierPoints(points);

    Coordinates2d::Draw(list);

    Coordinates2d::Wait();

    return 0;
}

NOTE: I am in love with my Collection<T> class, which is a wrapper around std::vector.

I am repeating much of what is already in the comments but felt it should be spelled out more explicitly.

Avoid using #define statements

Per Scott Meyers's Effective C++ you should always prefer const to #define statements because #define statements are a preprocessor step, which means when you are debugging, you will see a number and not a variable name. This can make your life very difficult, so you should avoid #defines whenever possible.

Descriptive Naming

You should only use a name like temp when it's really important to emphasize that you are using a temporary variable (say to draw your reader/client's attention to the fact that a function creates a temp object and so should be used sparingly when certain memory concerns are present). Quite literally any other name would be better than temp possibly even var. Why not use pointCollection or something similarly descriptive?

Comments where helpful

I don't see any comments in your code. I am also not familiar with the libraries you are using. If you want a reader like me to understand your code, you should put in some clarification, succinct one line statements should be enough. Even for yourself, you should put in a note about what you hoped to accomplish in a function.


AngularJS services for APIs in large applications I've been reading about AngularJS services and factories, and how you should strive for a thin controller, but as the project I've been working on grows it seems more and more impractical with as interconnected as the app is.  For a little background, I am using UI-Grid to display most data in the app (20+ grids) and MVC5.  At the moment, I've placed all of my API calls to SQL within the main controller, along with all of my grid definitions:

app.controller('MainController', ['$scope', '$http', '$filter', 'logger', function ($scope, $http, $filter, logger) {

    $scope.project = {};

    /*
    * Grid Listing Definition
    */
    $scope.gridListing = { enableRowSelection: true, enableRowHeaderSelection: false, multiSelect: false, enableColumnMenus: false, enableSorting: false };
    $scope.gridListing.onRegisterApi = function (gridApi) {
        // Set api on scope
        $scope.gridListing.Api = gridApi;
        gridApi.selection.on.rowSelectionChanged($scope, function (row) {
            $scope.gridListing.selectedRow = false; // Used for visibility on the view
            if (row.isSelected) {
                $scope.gridListing.selectedRow = row.entity;
            };
        });
    };
    $scope.gridListing.columnDefs = [
        { field: 'ListingNumber', displayName: 'Number', type: 'number', width: '10%' }
        , { field: 'CreatedOn', displayName: 'CreatedOn', type: 'date', cellFilter: 'date:"yyyy-MM-dd"', width: '10%' }
        , { field: 'CreatedBy', displayName: 'CreatedBy', width: '10%' }
        , { field: 'Description', displayName: 'Description' }
        , { field: 'FunctionState', displayName: 'State', width: '10%' }
    ];

    $scope.load_gridListing = function () { // Update when new Listing is selected
        $scope.gridListing.data = []; // Clear grid for new data
        $http.get('/api/qryListingAPI/?project=' + $scope.project.ProjectID)
        .then(function (response) {
            $scope.gridListing.data = response.data;
        })
        .catch(function (response) {
            // Custom logger for error tracking
            logger.log(usrName, 'app', response.status + " - " + response.statusText, logger.Error);
        });
    };
}]);

As the user interacts with objects in the view, like selecting a new project etc. various grids need to be updated to reflect the users selection.  I had tied these "load" functions on the view to update grids as needed.  I had originally thought of creating services for each of the views that I use to populate these grids, but with so many, and needing to share between controllers, I elected not to to avoid massive controller definitions and added complexity making sure each controller was on the same page.  I may be misunderstanding how to implement AngularJS the way many say AngularJS should be implemented and making it more complicated than it needs to be.  Any suggestions for implementing an app with 20+ grids and 30+ APIs?

it seems more and more impractical with as interconnected as the app is

You mentioned 30+ APIs. Let's start with that. If all of a sudden, someone from the API team decides to change the semantics for several of the APIs, where would you be starting your end of the change?

Another scenario would be if your manager told you to scrap your code because you'll have to start a new project... using the same API. You'll of course throw away the template and controller because they're app-specific. But the way you wrote your code, you'll also be throwing away all your API. Nothing is spared.

The Angular Way™ of doing this is that anything data-related should live in factories/services while anything UI related lives in the controller, templates and directives. Reasons for doing so includes:

You write the data source just once.
You can easily share data sources across different controllers.
You can easily throw away the UI layers without involving the data layer.

I normally start writing Angular apps by defining my data sources. Once that's done, I build the UI layers and just pull in the dependencies I need. If I want user info in this controller as well as another controller, I don't have to write 2 AJAX calls. All I would need is to pull in the same factory and call the same function in both controllers.

In your app, start by writing every single one of those APIs in its own factory . This means the following code should be gone from the controller:

$http.get('/api/qryListingAPI/?project=' + $scope.project.ProjectID)

Now if you were thinking of writing every single API as an $http call, that's where you got it wrong. Angular has this thing called $resource. Hand it a REST-compliant endpoint and it provides you an object with REST methods. A factory could even be as simple as:

app.factory('QueryListing', function(){
  return $resource('/api/qryListingAPI/');
});

If you did it with factories, no matter how many grids you have, you only need to write the API just once in the service, and just pull in the service. Additionally, resources expose promises. You can easily pop in a then.

app.controller('MainController', function($scope, ..., QueryListing){
  QueryListing.get(...).$promise.then(function(){
    ...
  });
});


VB6: String Searching Performance 5 years ago, when i was in love with VB, I created this simple program to detect if a file is virus or not.

What I'd like is to see if you can help me optimize it, speed-wise.

I open an executable file in a rich textbox control and use the Instr method to search for occurrences of predefined string. The problem is that the Instr method is damn slow when searching files as small as 512KB.

Can the speed be enhanced?

If InStr(1, rt.Text, "scripting.filesystemobject") Then
 It does something with FSO
End If

If InStr(1, rt.Text, "thisprogramcannotberunindosmode") Or InStr(1, rt.Text, "thisprogrammustberununderwin32") Then
  Probably a DOS program
 Else
  looks like a GUI program
End If

If InStr(1, rt.Text, "hkey_local_machine") Then
 Does something with registry
End If
If InStr(1, rt.Text, "hkey_users") Then
 Does something with registry
End If
If InStr(1, rt.Text, "hkey_current_user") Then
 Does something with registry
End If
If InStr(1, rt.Text, "hkey_classes_root") Then
 Does something with registry
End If
If InStr(1, rt.Text, "hkey_current_config") Then
 Does something with registry
End If

In VB2010 the instr out performed indexof

    Dim ipsum As String = "Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."

    Dim tries As Integer = 100000
    Dim stpw As New Stopwatch
    Dim iof As Integer
    Dim f As String = "sint"

    stpw.Reset()
    stpw.Start()

    For x As Integer = 1 To tries
        iof = ipsum.IndexOf(f)
    Next
    stpw.Stop()
    Label1.Text = stpw.ElapsedMilliseconds.ToString("n2")

    stpw.Reset()
    stpw.Start()

    For x As Integer = 1 To tries
        iof = InStr(ipsum, f)
    Next
    stpw.Stop()
    Label2.Text = stpw.ElapsedMilliseconds.ToString("n2")


Searching an entire workbook for keywords and then printing the results on another worksheet I made a search page at the beginning of a work book. This page has a drop down that allows you to choose what category to search in, then you type your phrase or keyword into the search box and it finds all of the results. It then takes those results and pastes them in in the chart at the bottom of the search page. You then have the option to print out those results or clear the chart.  Pictures are also posted for better understanding.

Private Sub ComboBox1_Change()

End Sub

Private Sub ComboBox2_Change()
    UpdateSearchBox
End Sub

Private Sub CommandButton1_Click()
    Select Case TextBox1.Value
        Case "F"
            TextBox1.Value = "G"
        Case "E"
            TextBox1.Value = "F"
        Case "D"
            TextBox1.Value = "E"
        Case "C"
            TextBox1.Value = "D"
        Case "B"
            TextBox1.Value = "C"
        Case "A"
            TextBox1.Value = "B"
        Case "G"
            TextBox1.Value = "A"
    End Select
End Sub

Private Sub CommandButton2_Click()
    FindOne
End Sub

Private Sub TextBox1_Change()
    UpdateSearchBox
End Sub

Sub UpdateSearchBox()
    Dim PageName As String, searchColumn As String, ListFiller As String
    Dim lastRow As Long

    If TextBox1.Value <> "" Then
        PageName = TextBox1.Value
    Else
        Exit Sub
    End If

    Select Case ComboBox2.Value
        Case "EQUIPMENT NUMBER"
            searchColumn = "A"
        Case "EQUIPMENT NAME"
            searchColumn = "C"
        Case "DUPONT NUMBER"
            searchColumn = "F"
        Case "SAP NUMBER"
            searchColumn = "G"
        Case "SSI NUMBER"
            searchColumn = "H"
        Case "PART NAME"
            searchColumn = "I"
        Case ""
            MsgBox "Please select a value for what you are searching by."
    End Select

    lastRow = Sheets(PageName).Range("A65536").End(xlUp).Row

        If lastRow <> 0 And PageName <> "" And searchColumn <> "" Then
        ListFiller = PageName & "!" & searchColumn & "2" & ":" & searchColumn & lastRow
        ComboBox1.ListFillRange = ListFiller
    End If
End Sub
Sub FindOne()

    Range("B19:J1500") = ""

    Application.ScreenUpdating = False

    Dim k As Integer, EndPasteLoopa As Integer
    Dim myText As String, searchColumn As String
    Dim totalValues As Long
    Dim nextCell As Range

    k = ThisWorkbook.Worksheets.Count
    myText = ComboBox1.Value
    Set nextCell = Range("B20")
    If myText = "" Then
        MsgBox "No Address Found"
        Exit Sub
    End If

    Select Case ComboBox2.Value
        Case "EQUIPMENT NUMBER"
            searchColumn = "A"
        Case "EQUIPMENT NAME"
            searchColumn = "C"
        Case "DUPONT NUMBER"
            searchColumn = "F"
        Case "SAP NUMBER"
            searchColumn = "G"
        Case "SSI NUMBER"
            searchColumn = "H"
        Case "PART NAME"
            searchColumn = "I"
        Case ""
            MsgBox "Please select a value for what you are searching by."
    End Select

    For i = 2 To k
        totalValues = Sheets(i).Range("A65536").End(xlUp).Row
        ReDim AddressArray(totalValues) As String

        For j = 0 To totalValues
            AddressArray(j) = Sheets(i).Range(searchColumn & j + 1).Value
        Next j

        For j = 0 To totalValues
            If (myText = AddressArray(j)) Then
                EndPasteLoop = 1
                If (Sheets(i).Range(searchColumn & j + 2).Value = "") Then EndPasteLoop = Sheets(i).Range(searchColumn & j + 1).End(xlDown).Row - j - 1
                For r = 1 To EndPasteLoop
                    Range(nextCell, nextCell.Offset(0, 8)).Value = Sheets(i).Range("A" & j + r, "I" & j + r).Value
                    Set nextCell = nextCell.Offset(1, 0)
                Next r
            End If
        Next j
    Next i

    Application.ScreenUpdating = True
End Sub

Here are some of the changes I'd make

Use Option Explicit at the top of the module

Eliminate all errors found by compiling the code (Debug -> CompileVBA Project)
(declare all variables, first of all)

Remove the empty Sub ComboBox1_Change() declaration if you don't inded to use it
Replace string comparisons such as PageName <> "" with

PageName <> vbNullString (faster)
Len(PageName) > 0 (even faster)

As pointed out in the comment, generalize the lastRow and totalValues statements

From:

  lastRow = Sheets(PageName).Range("A65536").End(xlUp).Row

To

With Sheets(PageName)
    lastRow = .Range(.Rows.Count, 1).End(xlUp).Row
End With

To reduce total lines of code and make it a bit easier to maintain I'd replace the three Select statements with one separate Function, and use it like this:

Private Sub CommandButton1_Click()
    TextBox1.Value = GetSelectedLetter(TextBox1.Value)
End Sub

Sub UpdateSearchBox()
    ...
    searchColumn = GetSelectedLetter(ComboBox2.Value)
    If InStr(searchColumn, "Please") > 0 Then MsgBox searchColumn
    ...
End Sub

Sub FindOne()
    ...
    searchColumn = GetSelectedLetter(ComboBox2.Value)
    If InStr(searchColumn, "Please") > 0 Then MsgBox searchColumn
    ...
End Sub

Public Function GetSelectedLetter(ByVal sel As String) As String
    Select Case sel
        Case "G", "EQUIPMENT NUMBER":   GetSelectedLetter = "A":    Exit Function
        Case "B", "EQUIPMENT NAME":     GetSelectedLetter = "C":    Exit Function
        Case "E", "DUPONT NUMBER":      GetSelectedLetter = "F":    Exit Function
        Case "F", "SAP NUMBER":         GetSelectedLetter = "G":    Exit Function
        Case "D":                       GetSelectedLetter = "E":    Exit Function
        Case "SSI NUMBER":              GetSelectedLetter = "H":    Exit Function
        Case "PART NAME":               GetSelectedLetter = "I":    Exit Function
        Case "C":                       GetSelectedLetter = "D":    Exit Function
        Case "A":                       GetSelectedLetter = "B":    Exit Function
        Case vbNullString:
            GetSelectedLetter = "Please select a value for what you are searching by."
    End Select
End Function

(not the standard indentation, but I find it visually easier to work with)

Performance

One improvement: start the search after moving all data to arrays (array of arrays specifically)

Example:

Public Sub SearchAllSheetsArrayOfArrays()
    Const SRC_VAL = "Test"

    Dim ws As Worksheet, arr() As Variant, i As Long, j As Long, tc As Long, tw As Long
    Dim r As Long, c As Long, lbr As Long, ubr As Long, lbc As Long, ubc As Long

    i = 1
    tw = ThisWorkbook.Worksheets.Count
    ReDim arr(i To i + tw - 1)  'Place all data from each sheet in Variant sub-arrays

    For Each ws In ThisWorkbook.Worksheets
        If ws.UsedRange.Cells.Count > 1 Then
            arr(j + 1) = ws.UsedRange
            j = j + 1
        End If
    Next

    tw = j
    ReDim Preserve arr(i To tw) 'Trim the main Array if there are empty sheets

    For i = 1 To tw     'tw = total worksheets
        lbr = LBound(arr(i), 1): ubr = UBound(arr(i), 1)    'dimension 1 (rows)
        lbc = LBound(arr(i), 2): ubc = UBound(arr(i), 2)    'dimension 2 (cols)
        For r = lbr To ubr
            For c = lbc To ubc
                If Len(arr(i)(r, c)) > 0 Then
                    If InStr(arr(i)(r, c), SRC_VAL) > 0 Then tc = tc + 1 'collect vals
                End If
            Next
        Next
    Next
    Debug.Print tc
End Sub

Ps. I used very short variable names to make the code fit in its window without scroll bars, but more descriptive names should be used - like in your posted code, but try to keep naming conventions consistent: all variables start with lower case letters, all procedures start with upper case letters


SICP - exercise 1.11 - tree recursion From SICP

  Exercise 1.11: A function \$f\$ is defined by the rule that:

  \$f(n) = n\$ if \$n < 3\$, and

  \$f(n) = f(n-1)+2f(n-2)+3f(n-3)\$ if \$n >= 3\$.

  Write a procedure that computes f by means of a recursive process.
  Write a procedure that computes f by means of an iterative process.

Please review my code.

I think the recursive process is obvious enough but I put it here just in case it could be improved.

(define (f n)
  (if (< n 3) n
        (+ (f (- n 1))
                 (* 2 (f (- n 2)))
                 (* 3 (f (- n 3))))))

Now, here's the iterative process. I followed the relationship created by the fibonacci example. The idea is:

\$a = f(n+2)\$
\$b = f(n+1)\$
\$c = f(n)\$

(define (f n)
  (f-iter 2 1 0 n))

(define (f-iter a b c count)
  (if (= count 0)
      c
      (f-iter (+ a (* 2 b) (* 3 c)) a b (- count 1))))

How to make this code better and faster? Are there more efficient ways?

Before looking for efficiency, note that there is a logical error in the tail recursive version of your function: it does not terminate on negative values (while the first function does terminate).

A way of correcting this error is simply to perform the check before calling f-iter. For instance:

(define (f n)
  (if (< n 3)
      n
      (f-iter 2 1 0 n)))

(define (f-iter a b c count)
  (if (= count 0)
      c
      (f-iter (+ a (* 2 b) (* 3 c)) a b (- count 1))))

From the efficiency point a view, I performed a few (unscientific) tests in DrRacket and noticed the following facts:

If the auxiliary function is defined inside f, the execution is 30-35% faster (and I think this is also stylistically better!)
An additional speedup of 20-40% can be gained if we reverse the counter, making it starting from 0 and going to n.

The differences in execution times where noticeable with n greater than 15000.

And so this is the more efficient version according to those tests:

(define (f n)
  (define (f-iter a b c count)
    (if (= count n)
        c
        (f-iter (+ a (* 2 b) (* 3 c)) a b (+ count 1))))
  (if (< n 3)
      n
      (f-iter 2 1 0 0)))

Of course these differences can be accounted only to the compiler, not to a change of the algorithm, so that by using another compiler maybe no differences could be found, or they could even be reversed!


Checking if an integer permutation is cyclic in Java A an integer permutation array[] is said to be cyclic if and only if we can choose an arbitrary element at position i, move it to the array[i]'s position, move the array[i]'s element to array[array[i]]'s position, and finally end up to the ith position. For example, \$\langle 1, 2, 0 \rangle\$ is cyclic, whereas \$\langle 1, 0, 2 \rangle\$ is not since if we start from 2, we sill end up in an infinite loop. My attempt is as follows:

package net.coderodde.math;

import java.util.Objects;

/**
 * This class checks that a given permutation consists of a single cycle
 * covering the entire sequence. The idea is that if the input array is
 * {@code 1, 0, 2}, it returns {@code false} since 2 is in its correct position.
 * On the other hand, {@code 1, 2, 0} returns {@code true} since it consists of
 * only one permutation cycle.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Feb 22, 2019)
 */
public class PermutationCyclicity {

    private static final String NUMBER_OUT_OF_BOUNDS_EXCEPTION_FORMAT =
            "array[%d] = %d, array.length = %d";

    public boolean isCyclic(int[] array) {
        check(array);
        int nextNumber = array[array[0]];
        int visitedNumbers = 1;

        while (nextNumber != array[0]) {
            nextNumber = array[nextNumber];
            visitedNumbers++;
        }

        return visitedNumbers == array.length;
    }

    private void check(int[] array) {
        checkArrayHasLength(array);
        checkArrayContentsWithinBounds(array);
        checkArrayElements(array);
    }

    private void checkArrayHasLength(int[] array) {
        Objects.requireNonNull(array, "The input array is null.");

        if (array.length == 0) {
            throw new IllegalArgumentException("The array has length zero.");
        }
    }

    private void checkArrayContentsWithinBounds(int[] array) {
        for (int i = 0; i < array.length; i++) {
            int num = array[i];

            if (num < 0 || num >= array.length) {
                String exceptionMessage =
                        String.format(NUMBER_OUT_OF_BOUNDS_EXCEPTION_FORMAT,
                        i,
                        num,
                        array.length);

                throw new IllegalArgumentException(exceptionMessage);
            }
        }
    }

    private void checkArrayElements(int[] array) {
        int[] counts = new int[array.length];

        for (int i : array) {
            if (++counts[i] > 1) {
                throw new IllegalArgumentException(
                        "The value " + i + " appears more than once.");
            }
        }
    }

    public static void main(String[] args) {
        PermutationCyclicity pc = new PermutationCyclicity();
        System.out.println(pc.isCyclic(new int[] { 1, 2, 3, 0 }));

        try {
            pc.isCyclic(new int[] { 1, 0, 1 });
        } catch (Exception ex) {
            System.out.println(ex.getMessage());
        }

        try {
            pc.isCyclic(new int[] { 1 });
        } catch (Exception ex) {
            System.out.println(ex.getLocalizedMessage());
        }

        System.out.println(pc.isCyclic(new int[] { 1, 0, 2 }));
    }
}

Critique request

I would like to hear comments regarding exception throwing and naming conventions, yet feel free to tell me anything that comes to mind.

Here

int nextNumber = array[array[0]];
int visitedNumbers = 1;

while (nextNumber != array[0]) {
    nextNumber = array[nextNumber];
    visitedNumbers++;
}

you determine the length of the orbit starting array[0]. If you start at 0 instead then some array lookups are saved, and I find it easier to understand:

int nextNumber = array[0];
int visitedNumbers = 1;

while (nextNumber != 0) {
    nextNumber = array[nextNumber];
    visitedNumbers++;
}

And with a do-while loop the initial array lookup can be made part of the loop:

int currentNumber = 0;
int visitedNumbers = 0;

do {
    currentNumber = array[currentNumber];
    visitedNumbers++;
} while (currentNumber != 0);

Further suggestions:

Checking for duplicate array elements is not necessary if you limit the number of iterations:

int currentNumber = 0;
int visitedNumbers = 0;

do {
    currentNumber = array[currentNumber];
    visitedNumbers++;
} while (visitedNumbers <= array.length && currentNumber != 0);

This saves one array traversal and the additional storage in checkArrayElements(). The only downside is that calling the function with an invalid permutation now possibly returns false instead of throwing an error.

Similarly, the “bounds check” can be made directly in that loop. That might be viewed as a violation of the “separation of concerns” principle, but saves one array traversal.