Type that returns a result based on some logic I have a type that returns a result based on some logic. The result contains a message. Should a good unit test check that the message is correct?

A negligible amount has changed as I'm not allowed to post IP:

class MyFilter
{
    private readonly int _state;

    public Result FilterResult { get; private set; }

    public MyFilter(int value)
    {
        _state = value;
    }

    public void Initialise()
    {
        string message;

        if (_state == 0)
        {
            message = "filtered because zero";
            FilterResult = new Result {ShouldFilter = true, Reason = message};
            log(message);
        }
        else if(_state == 1)
        {
            message = "filtered because one";
            FilterResult = new Result {ShouldFilter = true, Reason = message};
            log(message);
        }
        else
        {
            message = "not filtered because...";
            FilterResult = new Result {ShouldFilter = false, Reason = message};
            log(message);
        }
    }

    public bool ShouldFilter()
    {
        return FilterResult.ShouldFilter;
    }

    private void log(string message) { }
}

I want to test this type.  I want to test that it correctly sets the ShouldFilter flag on the result.  But I also want to test why it thinks it should filter or not, so I'm checking the string on the result too:

    [Test]
    public void When_zero()
    {
        MyFilter sut = new MyFilter(0);
        sut.Initialise();
        Assert.AreEqual(true, sut.FilterResult.ShouldFilter);
        Assert.AreEqual("filtered because zero", sut.FilterResult.Reason);
    }

I had some feedback on an internal code review saying that testing strings was a code smell;  if someone changes the string, the test will fail even though the behaviour may still be the same.

I personally think that, in this case, it's OK to test the string: the fact that the type thinks something is filtered is just part of the answer; I want to know why it thinks it should be filtered.

That's the crux of the question. It's not my ideal style of code, so, to give a little context, I'll explain what the code did before I inherited it:

The filter just used to log the reason why it was filtering. FilterResult didn't exist.  The code was just this:

class MyFilter
{
    private readonly int _state;
    private bool _filtered;

    public MyFilter(int value)
    {
        _state = value;
    }

    public void Initialise()
    {
        if (_state == 0)
        {
            _filtered = true;
            log("filtered because zero");
        }
        else if(_state == 1)
        {
            _filtered = true;
            log("filtered because one");
        }
        else
        {
            _filtered = false;
            log("not filtered because...");
        }
    }

    public bool ShouldFilter() => _filtered;

    private void log(string message) { }
}

This type was hard to test. The logic could change for the worse and it would be likely that the tests would still pass as they were just checking the binary ShouldFilter().

What I would've liked to have done, and what I normally do for important 'messages', is inject an INotifySupport interface as a dependency (have used this for years since reading this fine book), and rather than just logging these messages, I would call _notifySupport.OfSomethingFilterRelated(reason). I could then write a test named liked SupportWereNotified.

But I couldn't do that this time, so I exposed the Result property.

Anyway, bit of a convoluted post, so I'd better reiterate the question:

Should a good unit test check that the message is correct?

The problem is the Initialise method.

'Initialise', 'Init', 'PostConstruct', ... are always a code smell. All these methods perform actions/checks on an object after it has been created.
But most of the time, these actions/checks need to happen before, or while an objects is created.

Refactor the code by using a factory-method or builder where you put the 'initialise' code in the method/class, instead of in the object itself.

Then you can write an easy test on this object creation or validation method/class.

I would rewrite to something like this:

enum Reason{
    ZERO,ONE,OK
}

class MyFilterCreator
{
    public MyFilter create(int value)
    {
        if(shouldFilter(value) == Reason.OK){
            return new MyFilter(value);
        }else{
            return null;
        }
    }

    // write a test for this method
    public Reason shouldFilter(int _state)
    {
        if (_state == 0)
        {
            return Reason.ZERO;
        }
        else if(_state == 1)
        {
            return Reason.ONE;
        }
        else
        {
            return Reason.OK;
        }
    }
}

But to answer your question: No, do not check on messages, but on statuses.


PHP creating username with random number Trying to complete an add user script. Nothing out of the ordinary, however I have transitioned over to using prepared statements, and I am trying to get better at creating my own functions.

<?php
include("../include/sessions.php");

if(isset($_POST['criteria']))
{
  $value = $_POST['criteria'];
  $firstname = htmlspecialchars(trim($value['firstname']));
  $lastname = htmlspecialchars(trim($value['lastname']));
  // few more

  // set blank variable to be passed into function
  $dbusername = '';

  // pass necessary variables into the function
  // $dbc is the database connection needed for the prepared statement
  function createUsername($firstname, $lastname, $dbusername, $dbc)
  {
    // check if $user ever matches $dbusername, if so, rerun loop
    while($user == $dbusername)
    {
      $digits = 4;
      $randomNumber = rand(pow(10, $digits-1), pow(10, $digits)-1);
      // get first 3 letters of firstname
      $first3 = substr($firstname, 0, 3);
      // get first 3 letters of lastname
      $last3 = substr($lastname, 0, 3);
      // set user to $first3, $last3, and $randomNumber
      $user = $first3 . '' . $last3 . '' . $randomNumber;

      // prepare statement
      $select = "SELECT username FROM users WHERE username = ?;";
      $stmt = $dbc->prepare($select);

      // pass in the new $user
      $stmt->bind_param("s", $user);
      $stmt->execute();
      $result = $stmt->get_result();
      $row = $result->fetch_assoc();

      // if the created user happens to exist in the database, set it to $dbusername
      $dbusername = $row['username'];
      // rerun the loop if this happens
    }
    // if no matches, return the new username
    return $user;
  }
}

$user = createUsername($firstname, $lastname, $dbusername, $dbc);

echo 'username is ' . $user; // username should look like this: johbea3647
?>

I tried to notate the code as much as possible. But just in case, I'll break it down here...

The idea is to take create a brand new username that begins with the first 3 letters of the first and last name, then add a random 4 digit number to the end.

Hopefully, as long as the seemingly random username does not exist in the database, then return the new username. However, in the event the random username happens to exist, then run the loop again until there is no match.

I hope this all makes sense. Please review my code and see if there are any problems, or if there is a way to possibly simplify it all.

So far, everything seems to work. I'm just not too sure about that while loop.

I don't write PHP, but there are several general things I would suggest.

Your 2-space indent makes your code more difficult to read, I would recommend 4.

$digits is always 4, so I would declare it as a constant, and rename it accordingly. You can move it outside the while loop also.

const $NUMBER_OF_DIGITS = 4;

$first3 and $last3 are also not going to change, so I would move them out of your while loop, and also name them accordingly.

const $NUMBER_OF_DIGITS = 4;
const $FIRST_3 = substr($firstname, 0, 3);
const $LAST_3 =  substr($lastname, 0, 3);
while($user == $dbusername) {
    // ...

Your comments point out what should be easy to deduct for anyone who writes code regularly. I would suggest to reserve comments for function/class documentation (for example using PHPDoc) and otherwise, just to document things that are not obvious, or may be unusual workarounds to a certain problem/use case.

I think, in particular, perhaps this line deserves a short explanation:

$randomNumber = rand(pow(10, $digits-1), pow(10, $digits)-1);

Comments can also cause more work for you as the maintainer, as you have to also maintain comments in addition to the code, if anything is changed.

Comments can also be misleading, take this for example:

  // if the created user happens to exist in the database, set it to $dbusername
  $dbusername = $row['username'];
  // rerun the loop if this happens

The comment should really say something like:

  // set $dbusername to either NULL, or an existing username in the case it does exist, which will cause the loop to re-run if it already exists in the database

But that's already pretty obvious from the code, so I would just delete the comment instead, along with most or all of the other ones.

Lastly, I think the while loop works fine, but a do-while loop would make a bit more sense, in that it makes it clear that the loop should run at least once, but may run more times under certain conditions.

do {
    // your logic code as it is
} while($user == $dbusername);


URL-finder using a lot of memory I'm trying to get the URL of the browser with Mac OS X app. I wrote some AppleScript and am trying to use it in Cocoa. The problem is, when I watch it with instruments, memory is increasing, and at the end of 3-4 hours it's nearly 20MB.

First generation

Second generation (5 min. later)

#import "AppDelegate.h"
#import <Carbon/Carbon.h>

@interface AppDelegate ()
@property (weak) IBOutlet NSWindow *window;
@end

@implementation AppDelegate

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification {

    [NSTimer scheduledTimerWithTimeInterval:1
                                     target:self
                                   selector:@selector(collect)
                                   userInfo:nil
                                    repeats:YES];
}

- (void)collect{
    [self runWithEvent];
}

- (void)runWithEvent{
    NSURL *URL = [[NSBundle mainBundle] URLForResource:@"frontmostapptitle" withExtension:@"scpt"];
    if (URL) {
        NSAppleScript *appleScript = [[NSAppleScript alloc] initWithContentsOfURL:URL error:NULL];

        NSAppleEventDescriptor *returnDescriptor = [self lookUpRunningApp];

        NSDictionary *error = nil;
        NSAppleEventDescriptor *resultEventDescriptor = [appleScript executeAppleEvent:returnDescriptor error:&error];

        if (! resultEventDescriptor) {
            NSLog(@"%s AppleScript run error = %@", __PRETTY_FUNCTION__, error);
        }
        else {
            NSLog(@"%@", [self stringForResultEventDescriptor:resultEventDescriptor]);
        }
    }
}

- (NSAppleEventDescriptor *)lookUpRunningApp{
    // target
    ProcessSerialNumber psn = {0, kCurrentProcess};
    NSAppleEventDescriptor *target = [NSAppleEventDescriptor descriptorWithDescriptorType:typeProcessSerialNumber bytes:&psn length:sizeof(ProcessSerialNumber)];

    // function
    NSAppleEventDescriptor *function = [NSAppleEventDescriptor descriptorWithString:@"LookUpRunningApp"];

    // event
    NSAppleEventDescriptor *event = [NSAppleEventDescriptor
                                     appleEventWithEventClass:kASAppleScriptSuite
                                     eventID:kASSubroutineEvent
                                     targetDescriptor:target
                                     returnID:kAutoGenerateReturnID
                                     transactionID:kAnyTransactionID];
    [event setParamDescriptor:function forKeyword:keyASSubroutineName];
    return event;
}

- (NSString *)stringForResultEventDescriptor:(NSAppleEventDescriptor *)resultEventDescriptor
{
    NSString *result = nil;

    if (resultEventDescriptor) {
        if ([resultEventDescriptor descriptorType] != kAENullEvent) {
            if ([resultEventDescriptor descriptorType] == kTXNUnicodeTextData) {
                result = [resultEventDescriptor stringValue];
            }
        }
    }
    return result;
}

@end

- (NSString *)stringForResultEventDescriptor:(NSAppleEventDescriptor *)resultEventDescriptor
{
    NSString *result = nil;

    if (resultEventDescriptor) {
        if ([resultEventDescriptor descriptorType] != kAENullEvent) {
            if ([resultEventDescriptor descriptorType] == kTXNUnicodeTextData) {
                result = [resultEventDescriptor stringValue];
            }
        }
    }
    return result;
}

This method can be rewritten far more simply:

- (NSString *)stringForResultEventDescriptor:(NSAppleEventDescriptor *)resultEventDescriptor {
    return (resultEventDescriptor.descriptorType == kTXNUnicodeTextData) ? resultEventDescriptor.stringValue : nil;
}

- (void)collect{
    [self runWithEvent];
}

This method seems entirely unnecessary.  Why doesn't the timer just call runWithEvent directly?


Reading a text file, manipulating it and saving it to a mongo database We have an upcoming project at work which is going to require working with express.js. I have no prior experience with node.js, so I thought I'd try and do something aside from some courses. Are there any anti-patterns and what can be improved?

Objective

I have a text file that is retrieved from an external source and looks like this (It has a lot more lines than this)

I do not need the first 3 lines, but I do need the data separated by a "|". Wherever there is a "||" it adds an undefined value to the array so I just remove it. I also do not need any text which has a "/" in it.

2019-10-09 11:03:33 : ============================= ATTACK LOG ==============================
2019-10-09 11:03:33 : |                      -------  LOOT ------           ------ BONUS ------
2019-10-09 11:03:33 : |AC|TIME.|TROP.|SRC|DS|   GOLD| ELIXIR|  DE|TR.|S|  %|  GOLD|ELIXIR|  DE|L.
2019-10-09 11:15:44 :   1|11:15| 1761| 17| 4| 450447| 546344|1704|-17|0| 47|     0|     0|   0|--||211/220|
2019-10-09 11:20:49 :   1|11:20| 1744| 14| 4| 307817| 279805| 643| 10|1| 62| 15640| 15640|   0|G1||209/220|

I am using a library called line-reader to read the text file and my Mongoose schema is defined as Results

    router.get('/retrieve-logs', (req,res,next)=>{
      var headers = ['accountNumber','timeOfAttack',
                   'trophies','searches','ds',
                   'gold','elixir','darkElixir',
                   'trophyChange','victory','damagePercentage',
                   'bonusGold','bonusElixir','bonusDarkElixir'];

      const filePath = 'AttackLog-2019-10.log'

      //Read from each line
      lineReader.eachLine(filePath, (line)=> {
        //Do nothing if 'ATTACK LOG' in line
        if (line.includes('ATTACK LOG')) {
        } else {
          //Remove appended date and time
          removedDateTime = line.substring(22);

          if (removedDateTime.includes('GOLD')) {
          } else {
            //Grab lines after headers removed
            attackData = removedDateTime.split("|");
            tempAttackData = [];
            //Remove undefined from array of data and remove last value from array
            for(let i of attackData)
                i && tempAttackData.push(i);
            attackData = tempAttackData;
//Remove last item from array which we don't need
            attackData.pop();
            // Remove excess whitespace
            cleanedAttackData= [];
            attackData.forEach((data, index) => {
              cleaned = data.trim();
              cleanedAttackData.push(cleaned)
            });
            //Create object for DB {headers: attackData}
            var result = {};
            headers.forEach((headers, i) => result[headers] = cleanedAttackData[i]);

            //Save the created object
            finalResults = new Results(result)
            finalResults.save();
          }
        }
    })

      Results.find({},(err, docs) => {
        res.status(200).send({
          message: "Results successfully loaded",
          success: docs
        })
      })

    })

Style

Comments are just noise and thus reduce readability if they state what is obvious in the code. EG the comment //Read from each line is followed by the line
lineReader.eachLine(filePath, (line)=> {

Do not mix styles. You use semicolons randomly in the code. Use them or not, do not mix them

Do not add empty blocks or as your comment describes  "do nothing"  . Example if (line.includes('ATTACK LOG')) { } else  should be if (!line.includes('ATTACK LOG')) {

Do not add magic numbers or string in the code. Define them as constants in one place

Always delimite statement blocks with {}. EG for(let i of attackData) i && tempAttackData.push(i); should be  for(const i of attackData) { i && tempAttackData.push(i) }

Use constants for variables that do not change

Single argument arrow functions do not need the () around the argument. EG (line)=> { can be line => {

Spaces between commas

Undeclared variable are dangerouse. The variables finalResults, attackData, tempAttackData, cleanedAttackData and removedDateTime are all undecalared and thus are part of the top level scope and may clash creating very hard to debug problems

Avoid single use variables. EG  finalResults = new Results(result); finalResults.save(); can be new Results(result).save();

i is used as a loop counter and should not be used to represent a value or item of an array. The loop for (let i of ... should be for (const item of...

Do not add arguments to a function if that argument is not used. EG tempAttackData.forEach((data, index) => You do not use index so there is no need to define it

Logic
The whole thing feels clumsy as you move data from array to array, variable to variable, and step over and into nested and superfluous statements blocks.
It can be greatly simplified by creating a few helper functions to separate the various tasks. See example.
There also seams to be a bug as you do not check if the line is the second header line (contains "---- LOOT -----").
Example
The example breaks the task into smaller functions reducing the overall source code (and executional) complexity.
The additional item at the end of the data line is just ignored (rather than popped from the array) by the function Attack which creates the attack object you save.
const LOGS = "/retrieve-logs";
const message = "Results successfully loaded";
const FILE_PATH = "AttackLog-2019-10.log";
const FIELDS = "accountNumber,timeOfAttack,trophies,searches,ds,gold,elixir,darkElixir,trophyChange,victory,damagePercentage,bonusGold,bonusElixir,bonusDarkElixir";
const Attack = new Function(...FIELDS.split(","), "return {" + FIELDS + "};");

const valid = line => ! ["ATTACK LOG", "GOLD", "LOOT"].some(mark => line.includes(mark));
const clean = line => line.slice(22).replace("||", "|");
const parse = line => Attack(...clean(line).split("|").map(item => item.trim()));

router.get(LOGS, (req, res) => {
    lineReader.eachLine(FILE_PATH, line => { valid(line) && new Results(parse(line)).save() });
    Results.find({}, (err, success) => { res.status(200).send({message, success}) });
});


Faster Django CSV generation for several million database entries I am writing a CSV file generator that's filtering through about seven million database entries (MySQL backend).  This part is especially slow and I was wondering if there is a way to make it much faster. I was thinking of wiring to a temp file first before serving it and then deleting the file.

The Station.header() returns a list with header names ['a','b','c'], etc.

Is there an advantage to this? Is there a better way?

def metadata_file(request):
    """Gets metadata for sensors"""
    if request.GET.has_key('all'):
        s101 = Station.objects.all().filter(fwy='101', dir='S',
                                    abs_pm__gte=420.80, abs_pm__lte=431.63)
        s280 = Station.objects.all().filter(fwy='280', dir='S',
                                    abs_pm__gte=41.16, abs_pm__lte=56.49)
        q = s101|s280
        response = HttpResponse(mimetype='text/csv')
        response['Content-Disposition'] = 'attachment; filename="all_stations.csv"'

        writer = csv.writer(response)
        writer.writerow(Station().header())
        for x in q:
            writer.writerow(x.line())
        return response

1. Introduction

By coincidence, I'm working on a similar problem right now, so here's a chance for me to write up an experiment I ran today, in the hope that it might prove useful. However, the solution presented below is far from ideal (see section 3).

2. The idea

Processing your query results through Django's ORM and then through csv.writer is time-consuming. You can speed things up by getting the database to write the query output directly to a file and then serving that file.

First, your database user needs to have FILE privilege in order to be able to write files:

mysql> GRANT FILE ON *.* TO 'user'@'host';

Then you need to run your query with MySQL's INTO OUTFILE clause. You can get the query you need from Django by calling .query.sql_with_params():

from django.db.models import Q
s101 = Q(fwy='101', dir='S', abs_pm__gte=420.80, abs_pm__lte=431.63)
s280 = Q(fwy='280', dir='S', abs_pm__gte=41.16, abs_pm__lte=56.49)
sql, params = Station.objects.filter(s101|s280).query.sql_with_params()

Add the INTO OUTFILE clause:

sql += ''' INTO OUTFILE %s
           FIELDS TERMINATED BY ','
           OPTIONALLY ENCLOSED BY '"'
           LINES TERMINATED BY '\n' '''

Pick a place to put the CSV:

import os.path
csv_path = os.path.join(csv_directory, csv_filename)

Run the query:

from django.db import connection
cursor = connection.cursor()
cursor.execute(sql, params + (csv_path,))

Send the file (using StreamingHttpResponse which is new in Django 1.5):

from django.http import StreamingHttpResponse
response = StreamingHttpResponse(open(csv_path), content_type='text/csv')
response['Content-Disposition'] = 'attachment; filename=' + csv_filename
return response

3. Analysis

In my test cases (about a million records) this is around ten times as fast as processing the data through csv.writer in Python.

But there are several problems, mostly related to security:

This depends on the MySQL server being on the same machine as the Django web server. You can imagine working around this by some kind of reverse proxying but it all gets rather complicated.
Granting FILE access to your MySQL user is dangerous because it means that a SQL injection attacker can read and write files on your disk. You might want to create a separate MySQL user just to generate these CSV files, and you'll definitely want to set MySQL's secure_file_priv variable.
Even if the attacker can't inject SQL, they may be able to cause your disk to fill up with these temporary files. You need a plan for how they are going to be deleted. (Maybe using the request_finished signal?)
You need to find somewhere safe to put them (a directory to which the MySQL database user has write access).

4. Update

I did some more timing experiments today, trying out five different ways of generating and downloading the CSV, summarized in the table below:

+------+-----------------------+-------+
|      |     through Django    |Direct |
|      |Streaming|Not streaming|from DB|
+------+---------+-------------+-------+
|ORM   |      630|          370|    N/A|
+------+---------+-------------+-------+
|No ORM|      190|          125|     58|
+------+---------+-------------+-------+

Notes on the table:

Times are in seconds.
This is for a query with a million rows that generates 150 MiB of CSV.
"ORM" means that I passed the query results through Django's object-relational mapping system before generating the CSV.
"No ORM" means that I avoided this by running a custom SQL query.
"Streaming" means that I used a StreamingHttpResponse and generated the CSV one line at a time using a technique I'll describe below.
"Not streaming" means that I used the technique described in the Django documentation to generate the CSV. (This involves reading the entire CSV output into memory, which is a bit painful for other processes running on the server.)
"Direct" is the technique described in section 2 above, using MySQL's INTO OUTFILE clause.

Clearly Django's ORM and Python's csv.writer are both significant bottlenecks. If only the INTO OUTFILE approach weren't so painful!

So I would still be interested to see other suggestions for speeding up this operation.

5. Appendix: streaming CSV download

Python's csv module doesn't provide an easy way to generate one record at a time, but you can coerce it into doing so like this, using io.BytesIO to capture the output:

from io import BytesIO

def writerow(row):
    """Format row in CSV format and return it as a byte string."""
    bio = BytesIO()
    csv.writer(bio).writerow([unicode(r).encode('utf-8') for r in row])
    return bio.getvalue()


Asynchronous query to load products from laravel controller The following code works fine. When a user clicks on an element, an AJAX request is made, which will return the query result. The results are being displayed in a view. I read many articles where everyone is doing HTML code in controller when getting data with AJAX and show in view as same I did. Any good suggestions to avoid HTML code in controller make controller view and script separate.

Controller:

public function ajax(Request $request){

// $data = $request->all();

$data['products'] = Product::select('products.id', 'products.name', 'products.banner')->get();

foreach ($data['products'] as $product){
    echo $product->name;
}

// dd($data);
// if($request->ajax()){
//     return "AJAX";
// }
// return "HTTP";

// $data['products'] = Product::select('products.id', 'products.name', 'products.banner')->get();
// return $data;

}

Script

view

    @foreach($leedManufacturers as $leedsManufacturer)
                    {{-- @foreach($leedManufacturers as $leedsManufacturer)  --}}

                            {{str_limit($leedsManufacturer['name'], 300)}}
                                    <input type="checkbox">

                        {{-- for load more script --}}
                    {{-- <input type="hidden" id="row" value="0"> --}}

                    {{-- <input type="hidden" id="all" value="{{$total_manufacturers}}"> --}}
                    @endforeach

Your example:

public function ajax(Request $request){
$data['products'] = Product::select('products.id', 'products.name', 'products.banner')->get();

foreach ($data['products'] as $product){
    echo $product->name;
}

Attn: I omit commented string.

You do not need to get the query results into $data, especially if before you perform $data = $request->all();. Use another variable, in this case the $products may be okay.

If your 'Product' model is correct, then in the query you not need to use products.id - i.e. table name, need just field name.

Next, when you want pass the result to view, you need to do so as described in Doc:

return view('your_view_name', ['products' => $products]);

As described in official documentation  - versions there are from 5.0 up to 5.8, choose any you need.

So, this part of your code becomes:

public function ajax(Request $request){
    $products = Product::select('id', 'name', 'banner')->get();
    return view('your_view_name', ['products' => $products]);
}

Remember 2 things:

With that query you'll get all products from DB and there may be a huge number of results.
the view has to be a blade type of template.

In your 'view' I see this line:

@foreach($leedManufacturers as $leedsManufacturer)

but I do not see a variable $leedManufacturers in the controller so you have no data for it.

To show $products result use the same but for $products, like:

    @foreach($products as $product)

id: {{$product->id}}, name: {{$product->name}}

{{$product->banner}}

    @endforeach

Also, when you need , you may use this (example)

@if (xxxxxxxxx === 1)
    I have one record!
@elseif (yyyyyyyyyy > 1)
    I have multiple records!
@else
    I don't have any records!
@endif

https://laravel.com/docs/5.3/blade#if-statements

And in view you can use even

@php
    pure php code here
@endphp


Changing the first occurrence of a text in a sentence The following question was taken from Absolute Java 5th ed. by Walter Savitch:

  Write a program that starts with a line of text and then outputs that line of text with the first occurrence of "hate"  changed to  "love" . For example, a possible sample output might be:
  The line of text to be changed is:
  I hate you.
  I have rephrased that line to read:
  I love you.
   You can assume that the word  "hate"  occurs in the input. If the word  "hate" occurs more than once in the line, your program will replace only the first occurrence of "hate" .  Use     a defined constant for the string to be changed. To make your program work for another string, you should only need to change the definition of this defined constant.

This is the code that I have written:

 public class Question5 {

    private static final String STRING_TO_BE_CHANGED = "hate";

    public static void main(String[] args) {

        System.out.println("The line to be changed is:");
        System.out.println(originalLine());
        System.out.println("I have rephrased that line to read:");
        System.out.println(newLine());
    }

    private static String originalLine() {
        return "I hate you";
    }

    private static String newLine() {
        return originalLine().replaceFirst(STRING_TO_BE_CHANGED, "love");
    }
}

This, and your past few questions you have asked, have been progressively getting better.

In particular, the naming of methods and variables in this question is much better. You are suffering from 'Hungarian Notation' in the constant name. There is no need to call it STRING_TO_BE_CHANGED because we know it is a String. A simpler name TO_CHANGE would be fine.

In order to take your programming mindset to 'the next level', you have to start to abstract the functions from the particular use case. How do you write your program to solve a general problem, and apply it to a specific use case?

In this program of yours, the guts of the function newLine() can only solve one problem, changing hate to love, in a single phrase.

What I would like to see is a more general use case. In this problem, you have three Strings:

I hate you
hate
love

The objective is to search the first string for the second, and to replace it with the third.

A general function would look like:

public static String replaceWordInText(String phrase, String search, String replacement) {...}

That function embodies the logic you are trying to do, and it can replace any search with any replacement in any text.

How would this be used to solve the problem you have in the book?

public class Question5 {

    private static final String TO_SEARCH = "I hate you";
    private static final String TO_CHANGE = "hate";
    private static final String REPLACE_WITH = "love";

    public static void main(String[] args) {

        System.out.println("The line to be changed is:");
        System.out.println(STRING_TO_BE_SEARCHED);
        System.out.println("I have rephrased that line to read:");
        String result = replaceWordInText(TO_SEARCH, TO_CHANGE, REPLACE_WITH);
        System.out.println(result);
    }

    public static String replaceWordInText(String phrase, String search, String replacement) {
        ....
    }

}

Now, we have a general function applied to a specific use-case. How would the general function be implemented? Well, you have already solved that using the replaceFirst method

    public static String replaceWordInText(String phrase, String search, String replacement) {
        return phrase.replaceFirst(search, replacement);
    }


Fast distance calculation for Starcraft2 bot I am coding a bot for Starcraft 2, in which many distances have to be calculated every frame.

Here is the part of the library that is being used and I want to improve: https://github.com/Dentosal/python-sc2/blob/develop/sc2/position.py

I built a new class Points that inherits from np.ndarray.
It is not yet connected to the rest of the library, but the functions are done. I removed the functions furthest_to, further_than and so on because the closer-versions are basically the same execpt a -1 or <.

Are these functions implemented in most efficient way? Is there a way to improve the parts that look like this:

find = np.where(np.any(M < distance, axis=1))
selection = np.array([self[i] for i in find[0]])

Any other comments or suggestions are also welcome :)

from typing import Any, Dict, List, Optional, Set, Tuple, Union  # for mypy type checking

import numpy as np
from scipy.spatial.distance import cdist

from position import Point2

class Points(np.ndarray):
    def __new__(cls, units_or_points):
        obj = np.asarray(units_or_points).view(cls)
        return obj

    def closest_to(self, point: Point2) -> Point2:
        """Returns the point of self that is closest to another point."""
        if point in self:
            return Point2(tuple(point))
        deltas = self - point
        distances = np.einsum("ij,ij->i", deltas, deltas)
        result = self[np.argmin(distances)]
        return Point2(tuple(result))

    def closer_than(self, point: Point2, distance: Union[int, float]) -> "Points":
        """Returns a new Points object with all points of self that
        are closer than distance to point."""
        position = np.array([point])
        M = cdist(self, position)
        find = np.where(np.all(M < distance, axis=1))
        selection = np.array([self[i] for i in find[0]])
        return Points(selection)

    def in_distance_between(
        self, point: Point2, distance1: Union[int, float], distance2: Union[int, float]
    ) -> "Points":
        """Returns a new Points object with all points of self
        that are between distance1 and distance2 away from point."""
        p = np.array([point])
        M = cdist(self, p)
        find = np.where(np.any(np.logical_and(distance1 < M, M < distance2), axis=1))
        selection = np.array([self[i] for i in find[0]])
        return Points(selection)

    def sort_by_distance_to(self, point: Point2, reverse: bool = False) -> "Points":
        """Returns a new Points object with all points of self sorted by distance to point.
        Ordered from smallest to biggest distance. Reverse order with keyword reverse=True."""
        deltas = self - point
        distances = (1 if reverse else -1) * np.einsum("ij,ij->i", deltas, deltas)
        result = self[distances.argsort()[::-1]]
        return Points(result)

    def closest_n_points(self, point: Point2, n: int) -> "Points":
        """Returns a new Points object with the n points of self that are closest to point."""
        deltas = self - point
        distances = np.einsum("ij,ij->i", deltas, deltas)
        result = (self[distances.argsort()[::-1]])[-n:]
        return Points(result)

    def in_distance_of_points(self, points: "Points", distance: Union[int, float]) -> "Points":
        """Returns a new Points object with every point of self that
        is in distance of any point in points."""
        M = cdist(self, points)
        find = np.where(np.any(M < distance, axis=1))
        selection = np.array([self[i] for i in find[0]])
        return Points(selection)

    def n_closest_to_distance(self, point: Point2, distance: Union[int, float], n: int) -> "Points":
        """Returns a new Points object with the n points of self
        which calculated distance to point is closest to distance."""
        deltas = self - point
        distances = np.absolute(distance - np.einsum("ij,ij->i", deltas, deltas))
        result = (self[distances.argsort()[::-1]])[-n:]
        return Points(result)

You should probably have another look at the scipy.spatial module. It provides (hopefully) faster methods for most of these checks using a k-d-tree.

from scipy.spatial import cKDTree

class Points(np.ndarray):
    def __new__(cls, units_or_points):
        obj = np.asarray(units_or_points).view(cls)
        obj.kd_tree = cKDTree(obj)
        return obj

    def closest_to(self, point: Point2) -> Point2:
        """Returns the point of self that is closest to another point."""
        _, i = self.kd_tree.query([[point.x, point.y]])
        return Point2(self[i][0])

    def closer_than(self, point: Point2, distance: Union[int, float]) -> "Points":
        """Returns a new Points object with all points of self that
        are closer than distance to point."""
        selection = self.kd_tree.query_ball_point([point.x, point.y], distance)
        return self[selection]

    def in_distance_between(
        self, point: Point2, distance1: Union[int, float], distance2: Union[int, float]
    ) -> "Points":
        """Returns a new Points object with all points of self
        that are between distance1 and distance2 away from point."""
        selection_close = self.kd_tree.query_ball_point([point.x, point.y], distance1)
        selection_far = self.kd_tree.query_ball_point([point.x, point.y], distance2)
        selection = list(set(selection_far) - set(selection_close))
        return self[selection]

    def closest_n_points(self, point: Point2, n: int) -> "Points":
        """Returns a new Points object with the n points of self that are closest to point."""
        _, indices = self.kd_tree.query([[point.x, point.y]], k=n)
        return self[indices]

    def in_distance_of_points(self, points: "Points", distance: Union[int, float]) -> "Points":
        """Returns a new Points object with every point of self that
        is in distance of any point in points."""
        pairs = self.kd_tree.query_ball_tree(points.kd_tree, distance)
        return points[[i for closest in pairs for i in closest]]

These are all the ones I could quickly find a way for using the tree. Not included are sort_by_distance_to, n_closest_to_distance and n_closest_to_distance.

In order to test if this is really faster, here are some tests, with the following setup:

np.random.seed(42)
points = np.random.rand(300, 2)
points_graipher = Points(points)
points_op = PointsOP(points)
point = Point2(np.random.rand(2))
points2 = np.random.rand(10, 2)
points2_graipher = Points(points2)

Here PointsOP is you class and Points is the class defined in this answer.

%timeit points_op.closest_to(point)
# 38.3 µs ± 1.35 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)
%timeit points_graipher.closest_to(point)
# 43.7 µs ± 249 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

%timeit points_op.closer_than(point, 0.1)
# 39.5 µs ± 238 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
%timeit points_graipher.closer_than(point, 0.1)
# 11 µs ± 26 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

%timeit points_op.in_distance_between(point, 0.1, 0.2)
# 52.9 µs ± 275 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
%timeit points_graipher.in_distance_between(point, 0.1, 0.2)
# 21.9 µs ± 180 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

%timeit points_op.closest_n_points(point, 10)
# 29.5 µs ± 359 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
%timeit points_graipher.closest_n_points(point, 10)
# 41.7 µs ± 287 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

%timeit points_op.in_distance_of_points(points2, 0.1)
# 116 µs ± 727 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
%timeit points_graipher.in_distance_of_points(points2_graipher, 0.1)
# 89.2 µs ± 500 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

As you can see for \$N = 300\$ points there are some methods which are faster with the KDTree (up to four times), some that are basically the same and some that are slower (by up to two times).

To get a feeling how the different approaches, scale, here are some plots. The only thing changing is the number of points. The steps are 30, 300, 3000, 30000.

To summarize, you should check this for some actual cases you have. Depending on the size of points your implementation or this implementation is faster.


Minesweeper, python Tkinter, Checking close by tiles Is there any smart way to compress the following code:

def __total(self, x, y):
    count = 0
    for x_offset in range(-1, 2):
        x_index = x + x_offset
        for y_offset in range(-1, 2):
            y_index = y + y_offset
            if 0 <= x_index < self.Size and 0 <= y_index < self.__Size:
                count += self.__buttons[y_index][x_index].mine
    if not count:
        for x_offset in range(-1, 2):
            x_index = x + x_offset
            for y_offset in range(-1, 2):
                y_index = y + y_offset
                if 0 <= x_index < self.__Size and 0 <= y_index < self.__Size:
                    self.__push(x_index, y_index)
    return count

I have been trying forever with an extreme "coder-blackout" not being able to succeed....

Okay. Improved my answer. You can call this code as normal with self.__total(x,y). In my own test, it showed the exact same results as your code has.

def __total(self, x, y, count=0, old=False):
    if (not count and old) or not old:
        for x_index in range(-1+x, 2+x):
            for y_index in range(-1+y, 2+y):
                if 0 <= x_index < self.__Size and 0 <= y_index < self.__Size:
                    if old:
                        self.__push(x_index, y_index)
                    else:
                        count += self.__buttons[y_index][x_index].mine

    if old:
        return count
    return self.__total(x,y, count, True)

Another solution would be using Numpy:

import numpy as np

def __total(self, x,y):
    count = np.sum( self.__buttons[ x-1 if x>0 else 0:x+2, y-1 if y>0 else 0:y+2 ] )
    if not count:
        for x_offset in range(-1, 2):
            x_index = x + x_offset
            for y_offset in range(-1, 2):
                y_index = y + y_offset
                if 0 <= x_index < self.__Size and 0 <= y_index < self.__Size:
                    self.__push(x_index, y_index)
    return count

The problem with this code is, that you can't use array class, but would need a Numpy array of all mines in a extra variable. But it would speed up your calculations just because of the nature of Numpy.


Strlen function optimization So I'm making a strlen function and I thought I would use SIMD instructions for it. It runs fine but when comparing it to std::strlen(), it falls a lot behind. Any ideas on how to make it faster would be much appreciated.
inline size_t strlen(const char* data) {
        const __m128i terminationCharacters = _mm_setzero_si128();
        const __m128i* pointer = (const __m128i*)data;

        size_t length = 0;

        const int compareMode = _SIDD_UBYTE_OPS | _SIDD_LEAST_SIGNIFICANT | _SIDD_CMP_EQUAL_EACH;

        for (;; length += 16, ++pointer) {
            const __m128i comparingData = _mm_loadu_si128(pointer);

            size_t index = _mm_cmpistri(terminationCharacters, comparingData, compareMode);

            if (index < 16)
                return length + index;
        }
    }
```

pcmpistri
This seems like the obvious choice searching withing a string. However, while pcmpistri is very general/powerful, it is also not very fast. On typical Intel processors it consists of 3 µops that all go to execution port p0 (therefore limiting this loop to at best running one iteration every 3 cycles), on AMD Zen(1/2) it's slightly less bad coming in at 2 µops and executing once every 2 cycles.
There is an in way more primitive way (just using SSE2) based on pcmpeqb and pmovmskb. That leaves you with a mask instead of an index, but for most of the loop that doesn't matter (all that matters is whether the mask is zero or not), and in the final iteration you can use tzcnt (or similar) to find the actual index of the zero byte within the vector.
That technique also scales to AVX2, which pcmpistri does not. Additionally, you could use some unrolling: pminub some successive blocks of 16 bytes together to go through the string quicker at first, at the cost of a more tricky final iteration and a more complex pre-loop setup (see the next point).
A bug
While an aligned load that contain at least one byte of the string is safe even if the load pulls in some data that is outside the bounds of the string (an aligned load cannot cross a page boundary), that trick is unsafe for unaligned loads. A string that ends near a page boundary could cause this function to fetch into the next page, and possibly trigger an access violation.
There are different ways to fix it. The obvious one is using the usual byte-by-byte loop until a sufficiently aligned address is reached. A more advanced trick is rounding the address down to a multiple of 16 (32 for AVX2) and doing an aligned load. There are bytes in it that aren't from the string, maybe including a zero. Therefore those bytes must be explicitly ignored, for example by shifting the mask that pmovmskb returned to the right by data & 15. If you decide to add unrolling, then the address for the main loop should be even more aligned, to guarantee that all the loads in the main loop body are safe.


Using static final Strings for SQL Query in Spring Boot This is regarding code quality and coding standards with respect to Spring Ecosystem. Here there are two methods:

isRecordExists is using a class level static constant String query. (Definitely suggestable for old way where we use prepared statements)
isRecordExists2 is using method level String query

Which method is better (isRecordExists or isRecordExists2) in context with  Spring Boot?

@Repository
public class SpringRepository {

    @Autowired
    private NamedParameterJdbcTemplate namedParamJdbcTemplate;

    private static final String QUERY = " select * from Dual where created_date=:date";

    public void isRecordExists(Date date){
       /**
       *  using constant for sql Query String at class level
       */
         MapSqlParameterSource parameters = new MapSqlParameterSource();
         parameters.addValue("date", date);
         namedParameterJdbcTemplate.queryForObject(QUERY, parameters, Integer.class);

    }
   public void isRecordExists2(Date date){
         String sqlQuery=" select * from Dual where created_date=:date";
       /**
       *  using  sql Query at method level
       */
         MapSqlParameterSource parameters = new MapSqlParameterSource();
         parameters.addValue("date", date);
         namedParameterJdbcTemplate.queryForObject(sqlQuery, parameters, Integer.class);

    }

Sorry to present the antithesis to an existing answer once more: if the string is neither reusable nor public, a constant makes no sense in my book. (And yes, this question is opinion-based ;-))

I see why you use a string-variable inside the medthod (to keep it readable for longer strings) which I find totally OK.

In general: aim to write the code for the audience of developers who come after you. In my experience, this future developer will dig through your code with a debugger because something goes wrong. Thus, he will not read the class in its entirity because he's curious, he will jump into that specific method from a stacktrace. All information that is locally there will be beneficial, as there is less jumping around in the code.


Remove and add connection string I want to remove a connection string that may already have been set in a different config file and then set it again.
Say my application has a Web.config file and a Web.Debug.config file then I can achieve what I want to do in the following way:
Web.config:
  <connectionStrings>
    <remove name="MyConnectionString" />
    <add name="MyConnectionString" connectionString="...";User Id=******;Password=******;" />
  </connectionStrings>

Web.Debug.config
  <connectionStrings>
    <remove xdt:Transform="Replace" name="MyConnectionString" />
    <add
      xdt:Transform="SetAttributes(connectionString)"
      xdt:Locator="Condition(@name='MyConnectionString')"
      connectionString="...";User Id=******;Password=******;"
    />
  </connectionStrings>

I am going to be removing connection strings and then adding them to ensure there are no errors that result from a connection string already having been set in some other config file. Before I change tens or hundreds of config files across all of our repositories I want to know if there is a better way - easier to read or requiring fewer/simpler changes.

Each add entry in connectionStrings section will be parsed as ConnectionStringSettings at the end of the day.
This class has 3 properties: Name,ConnectionString and ProviderName.
In your provided sample you have defined only the first two.
Your provided sample seems to me a bit too specific/strict:

Replace only the connectionStrings attribute
Match the entity where the name equals MyConnectionString

The same result could be achieved with less specific transformation rules:
<connectionStrings>
    <remove xdt:Transform="Replace" name="MyConnectionString" />
    <add name="MyConnectionString" connectionString="...;User Id=******;Password=123;"
      xdt:Transform="SetAttributes" xdt:Locator="Match(name)" />
  </connectionStrings>

I've specified the name as an attribute of the add element
I've changed the Locator attribute value from Condition to Match
And I've removed the (connectionString) suffix from the Transform attribute

I've checked the transformation with this tool.


Breadth and Depth First Search in Ruby I was tasked with building three individual methods; one to create a Binary Search Tree (BST), one to carry out a Breadth First Search (BFS), and one to carry out a Depth First Search (DFS).

Create Binary Tree

class Node
  attr_accessor :value, :left, :right

  def initialize(value)
    @value = value
  end
end

def build_tree(array, *indices)
  array.sort.uniq!
  mid = (array.length-1)/2
  first_element = indices[0]
  last_element = indices[1]

  if !first_element.nil? && first_element >last_element
    return nil
  end

  root = Node.new(array[mid])
  root.left = build_tree(array[0..mid-1], 0, mid-1)
  root.right = build_tree(array[mid+1..-1], mid+1, array.length-1)

  return root
end

Breadth First Search

def breadth_first_search(search_value, tree)
  queue = [tree]
  visited = [tree]

  while !queue.empty?
    current = queue.shift
    visited << current
    left, right = current.left, current.right

    if current.value == search_value
      puts current
      exit
    end

    if !left.nil? && !visited.include?(left)
      if left.value == search_value
        puts left
        exit
      else
        visited << left
        queue << left
      end
    end

    if !right.nil? && !visited.include?(right)
      if right.value == search_value
        puts right
        exit
      else
        visited << right
        queue << right
      end
    end
  end
  puts "nil"
end

Depth First Search

def depth_first_search(search_value, tree)
  stack = [tree]
  visited = [tree]

  while !stack.empty?
    current = stack.last
    left, right = current.left, current.right

    if current.value == search_value
      puts current
      exit
    elsif !left.nil? && !visited.include?(left)
      if left.value == search_value
        puts left
        exit
      else
        visited << left
        stack << left
      end
    elsif !right.nil? && !visited.include?(right)
      if right.value == search_value
        puts right
        exit
      else
        visited << right
        stack << right
      end
    else
      stack.pop
    end
  end
  puts "nil"
end

Calling the Methods

binary_tree = build_tree([4,7,2,8,1,1,1,30,22,4,9])

breadth_first_search(9, binary_tree)
depth_first_search(7, binary_tree)

Why do you have a build_tree function instead of a BST class with a constructor? It can just be a container to wrap around Node, but that way you can encapsulate things better. Generally, a BST isn't used because people want access to the tree structure, but because it's being used as a min heap or for quicker searching than O(n). If you really want to, you can expose the Nodes, but it'd still be better to wrap it in a class, if only so that...
...You can write your search functions as methods on the tree class. That would make more sense than passing in a parameter which is, essentially, self.

You could then make more convenience methods, like insert and remove, which would allow you to easily create trees from unsorted arrays and provide those methods to the caller.

Use a Set for your visited collection, unless you really like O(n), as opposed to O(1). lookup.
Likewise, use a Queue for your queue. That way you get O(1) enqueue and dequeue; with a normal array, one of the two is O(1), but the other is O(n) (depending on which end you enqueue and dequeue from)
I'd bet you can guess what I'm gonna say about your stack variable. But you're wrong! Haha, fooled you! Anyway, an array is fine for that, since (a) there's no Stack class in the Ruby standard library, at least as far as I'm aware, and arrays have O(1) push/pop if you use the methods with those names (which you do), as long as it never needs to resize to accommodate more elements. Even if it does, though, that happens relatively rarely; it's normally O(1).
Why are you putsing your return values instead of using those arrays you love so much? And why do you exit instead of returning? What if I want to run your example and see both outputs?
All in all, this code doesn't feel very Ruby-ish -- it seems more like a straight translation from the C++ equivalent. However, I can't think offhand of an elegant, straight-forward way to do the same thing, so I'm just going to leave you with this vague admonishment.
On second thought, scratch #6. I didn't really pay attention to what you were doing, but you shouldn't replace puts with return. Instead, if you reach the end, return false; if you find an equal element, return either true or the path you took to get there, if the path is that important.

It's worth noting that there's no point in making this a binary search tree if you're going to do a brute-force search every time. The point of a BST is that, at any given node, you can compare what you're looking for with the data of that node, and figure out if you have to go left or right or if you've found it, and thereby eliminate the need for breadth-first searching in general.

If you're only going to have B/DFS, just make it a generic tree, and don't bother with that fancy stuff about ordering and the like that you have in your "constructor". That'll be a better test, anyway, since a B/DFS is typically used in cases where you don't have ordering. If you really wanna keep the ordering, try writing a binary search; it's a fun little exercise.


Java function to format seconds as minutes, hours, days Anyone know how I write this code shorter, with modulos maybe? I also want to add weeks and years do this code, but it kind of got too long for that. Maybe there is some smart algoritm? Something in a for-loop?

public static String formatSeconds (long seconds)
{
    if (seconds < 60)
        return seconds != 1 ? seconds + " seconds" : "1 second";

    if (seconds < 3600)
    {
        seconds /= 60;
        return seconds != 1 ? seconds + " minutes" : "1 minute";
    }

    if (seconds < 86400)
    {
        seconds /= 3600;
        return seconds != 1 ? seconds + " hours" : "1 hour";
    }

    if (seconds < 2629743)  //month
    {
        seconds /= 86400;
        return seconds != 1 ? seconds + " days" : "1 day";
    }

    seconds /= 2629743;
    return seconds != 1 ? seconds + " months" : "1 month";
}

This is a case for an Enum and a NavigableMap.

TimeUnit.java

public enum TimeUnit {

    SECOND(1, "1 second", " seconds"),
    MINUTE(60, "1 minute", " minutes"),
    HOUR(3600, "1 hour", " hours"),
    DAY(86400, "1 day", " days"),
    MONTH(2629743, "1 month", " months");

    private final int SECONDS_COUNT;
    private final String ONE_UNIT;
    private final String MULTIPLE_SUFFIX;

    public TimeUnit(int secondsCount, String oneUnit, String multipleSuffix) {
        SECONDS_COUNT = secondsCount;
        ONE_UNIT = oneUnit;
        MULTIPLE_SUFFIX = multipleSuffix;
    }

    public getSecondsCount() {
        return SECONDS_COUNT;
    }

    public getOneUnit() {
        return ONE_UNIT;
    }

    public getMultipleSuffix() {
        return MULTIPLE_SUFFIX;
    }

}

TimeProcessor.java

public class TimeProcessor {

    private static final NavigableMap<Integer, TimeUnit> timePeriods = new TreeMap<>();
    static {
        for (TimeUnit unit : TimeUnit.values()) {
            timePeriods.put(unit.getSecondsCount(), unit);
        }
    }

    public static String toString(int secondsOfTime) {
        Map.Entry<Integer, TimeUnit> entry = timePeriods.floorEntry(secondsOfTime);
        if (entry == null || entry.getKey() == null) {
            return "";
        }

        int unitCount = secondsOfTime / entry.getKey();
        return toString(unitCount, entry.getValue());
    }

    public static String toString(int unitCount, TimeUnit unit) {
        return (unitCount == 1) ? unit.getOneUnit() : unitCount + unit.getMultipleSuffix();
    }

}

I find it easier to have all the data localized in one place, separate from the handling logic.  This makes it easier and more robust to add additional units like weeks, fortnights, seasons, years, decades, centuries, and millennia.

This is more verbose and self-documenting as well as easier to extend and maintain.  Each new unit is only a line of code.

When using if/else structures or ternary operators, I prefer to test positive conditions rather than negatives.  This avoids the inherent if not not logic.

When using either this version or your original, there is a bit of a problem in terms of how it handles months.  Not every month has the same length.  A month can be anywhere from twenty-eight to thirty-one days.  It might be better to make use of the existing date handling logic rather than spin your own.  Because months and larger units are not fixed length.  Even days can be problematic with daylight savings time.  I regard this more as a programming exercise than a solution to representing time intervals.

I haven't tested this, so beware of syntax or logical errors.


Is there a better way to implement this string builder? I have a function that returns a string that indicates if an element is the same as another element(s). The logic is:

function duplicates(duplicatesArray, id = 1) {
    var duplicates = '';

    var count = 1;

    var length = duplicatesArray.length;

    var _id = null;

    for (var i = 0; i < length; i++) {
        _id = duplicatesArray[i];

        if (_id !== id) {
            count++;

            if (length == 2) {
                duplicates = duplicates + ' ' + _id;

                return duplicates;
            } else if (count < length) {
                duplicates = duplicates + ' ' + _id;

                if (count + 1 < length) {
                    duplicates = duplicates + ',';
                }
            } else {
                duplicates = duplicates + ' and ' + _id;
            }
        }
    }

    return duplicates;
}

The above outputs:

console.log(
    'This item is the same as item ' + duplicates(
        [1, 2]
    )
); // This item is the same as item 2

console.log(
    'This item is the same as item ' + duplicates(
        [1, 2, 3]
    )
); // This item is the same as item 2 and 3

console.log(
    'This item is the same as item ' + duplicates(
        [1, 2, 3, 4]
    )
); // This item is the same as item 2, 3 and 4

This function takes in an id (here, I am defaulting it to id = 1 for the purpose of brevity) and it compares that id to elements in the first argument, which is an array of other IDs. The comments after the console.logs above is the expected output, I'm just wondering if there is a more succinct way of approaching this.

I really have difficulty understanding your explanation or your code, but the wanted output is clear. So going only by that I propose this function:

function duplicates(testArray, id = 1)
{
    return testArray.filter(testId => testId !== id);
}

the filter() method creates a new array with all elements that pass the test implemented by the provided function. In this case I test that the array items are unequal to the given id. The output of your tests is:

  This item is the same as item 2
  This item is the same as item 2,3
  This item is the same as item 2,3,4

So, there's not the fancy 'and' in there yet, but that can be added. Is it really needed? Anyway, adding a bit of extra code like this:

function formatArray(idArray)
{
    var ending = (idArray.length > 1) ? ' and ' + idArray.pop() : '';
    return idArray.join(', ') + ending;
}

function duplicates(testArray, id = 1)
{
    return formatArray(testArray.filter(test => test !== id));
}

where we join the array to a string separated by commas, except for the ending if there are multiple items, when we insert an 'and'. The thing that might not be obvious is the conditional (ternary) operator. If you don't like it you can replace it with a normal if (...) {...} else {...} statement. This returns:

  This item is the same as item 2
  This item is the same as item 2 and 3
  This item is the same as item 2, 3 and 4

Now this is "Code Review" so I should review your code. That is difficult. I can almost see what you were trying to do, but it is such a mess (sorry). It seems you started with the for loop and then just created this Frankenstein monster by adding bits. Inside the loop you have four ifs, two elses and even a return. Some of the actions are the same, like adding a space. I don't believe even you know exactly what's going on there. It might work, but that's it. This is not how you should be programming.

Some advice:

Try to analyse the problem before you start writing any code. There are two clear distinct jobs to do:

Get rid of the duplicates.
Generate the output.

Keep these two jobs apart. That makes each problem easier to solve and leads to easier to understand code, because the jobs themselves are easier. Ideally these two jobs should be done in two seperate functions. A function should only do one thing, and not two.

Now I have to agree that, what I did, simply requires a lot of experience. You need to know the language you're using. That will come with time and practice.

Another thing you need to pay attention to is the names you use. One of the major reason I cannot understand your code is that it is unclear what it all means. What is in duplicatesArray? And why is the function called duplicates? That just doesn't make much sense. Something like removeDuplicates would be better as a function name, because it implies a clear action.

In my personal opinion each function should always, and only, have one return in it. I'll admit that I don't always stick to this rule, but as a general idea it is sound. Having multiple returns makes a function harder to understand, and having no return just seems lazy. There's always something to return.


Shortest path via two intermediate points My code takes a long time for big graphs.

How can I make it faster?

I'm trying to calculate the shortest path for 3 path's using queue but for big input my program crashes.

I'm stuck here. Can someone help me?

def bfs(graph, start, end):
    queue = []
    queue.append([start])
    while queue:
        path = queue.pop(0)
        node = path[-1]
        if node == end:
            return len(path)
        for adjacent in graph.get(node, []):
            new_path = list(path)
            new_path.append(adjacent)
            queue.append(new_path)

def all_possiblities():
    possiblites = []
    for clinique in cliniques[0][1:]:
        for clinique2 in cliniques[1][1:]:
            path1 = bfs(sorted_connections, a, clinique)
            path2 = bfs(sorted_connections, clinique, clinique2)
            path3 = bfs(sorted_connections, clinique2, b)
            total_path = path1 + path2 + path3
            possiblites.append(total_path)
    return min(possiblites) - 2

if __name__ == '__main__':
    input_values = input().split()
    n, m = int(input_values[0]), int(input_values[1])
    a, b = int(input_values[2]), int(input_values[3])
    connections = [list(map(int, input().split())) for _ in range(m)]
    cliniques = [list(map(int, input().split())) for _ in range(m, m+2)]
    sorted_connections = {}
    for road in connections:
        city1, city2 = road
        sorted_connections.setdefault(city1, []).append(city2)
        sorted_connections.setdefault(city2, []).append(city1)
    print(all_possiblities())

Input:

10 12 4 6
0 1
1 2
2 7
7 6
6 9
9 8
8 4
4 3
3 0
5 1
5 4
5 6
2 0 2
2 8 9

Output:

8

The key reasons for the poor performance are:

Representing queue as a list. This is slow because popping an item off the beginning of a list takes time proportional to the length of the list.

What you need here is a collections.deque.
Computing shortest distances many times over. The shortest distance from a to clinique is computed again for every clinique2, even though it will be the same each time. Similarly, the shortest distance from clinique2 to b is computed again for every clinique.

What you need here is to compute these distances once and remember them.

If the number of cliniques is large compared with the number of cities, you should use the Floyd–Warshall algorithm to compute distances between all pairs of cities in one go (instead of using Dijkstra's algorithm many times, as in the posted code).

If I were implementing this, I'd use scipy.sparse.csgraph.floyd_warshall and write:

import numpy as np
from scipy.sparse.csgraph import floyd_warshall

def shortest_distance_via(graph, a, b, c1, c2):
    """Return the shortest distance in graph from a to b via C1 and C2 (in
    that order), where C1 and C2 are nodes in c1 and c2 respectively.

    Arguments:
    graph -- NxN array of distances between nodes.
    a, b -- nodes in the graph.
    c1, c2 -- arrays of nodes in the graph.

    """
    d = floyd_warshall(graph)
    ab = d[a, c1, np.newaxis] + d[np.meshgrid(c1, c2)] + d[np.newaxis, c2, b]
    return ab.min()


A storage bin thingy for Repl.it projects I have a Repl.it account that I use for my projects, and I don't really want to make a new api for storage every time, I've seen some like EasyDB and Jsonstore (I use Jsonstore in the background as storage here) but it doesn't have some of the features i want. It isn't done, but its working. I just wanted to see if there's any improvements I could make before I start working on the next few features.

from os import getenv as env
from flask import make_response
from flask import Flask, request
from flask_limiter import Limiter
from json_store_client import Client
from flask_httpauth import HTTPBasicAuth
from werkzeug.security import check_password_hash
from werkzeug.security import generate_password_hash

app = Flask('app')
auth = HTTPBasicAuth()
db = Client(env("TOKEN"))

limiter = Limiter(
    app,
    key_func=lambda: auth.username(),
    default_limits=["100 per second"]
)

@auth.verify_password
def verify_password(username, password):
    user = db.retrieve(f"users/{username}")
    try:
        if check_password_hash(user["password"], password):
            return True
    except: return False
    return False

@app.route('/regester', methods = ['POST'])
def register():
    if request.is_json: user = request.get_json()['username']
    else: user = request.form['username']
    if db.retrieve(f"users/{user}"):
        return False
    if request.is_json: db.store(f"users/{user}", { "password": generate_password_hash(request.get_json()['password'])})
    else: db.store(f"users/{user}", { "password": generate_password_hash(request.form['password'])})
    return make_response(user, 201)

@app.route('/', methods = ['GET'], defaults={'fullurl': ''})
@app.route('/<path:fullurl>', methods = ['GET'])
@auth.login_required
def index_get(fullurl):
    data = db.retrieve(f"data/{auth.username()}/{fullurl}")
    if data: return make_response(data, 200)
    return make_response({}, 204)

@app.route('/', methods = ['POST'], defaults={'fullurl': ''})
@app.route('/<path:fullurl>', methods = ['POST'])
@auth.login_required
def index_post(fullurl):
    if request.id_json: db.store(f"data/{auth.username()/{fullurl}}", request.get_json())
    else: db.store(f"data/{auth.username()}", request.form)
    return make_response(db.retrieve(f"data/{auth.username()}"), 201)

app.run(host="0.0.0.0")

Few optimization tips

verify_password function

avoid bare except: clause - at least use except Exception: (though it's also broad, check which exception class is most appropriate in that context)
a set of statements:

try:
    if check_password_hash(user["password"], password):
        return True
except: return False
return False

is an overloaded and verbose version of a more explicit logic:

@auth.verify_password
def verify_password(username, password):
    user = db.retrieve(f"users/{username}")
    try:
        return check_password_hash(user["password"], password)
    except Exception:
        return False

don't overuse (or avoid entirely) multiple statements one-liners like if <condition>: <long statement>
avoid writing duplicated lengthy db statements like:

if request.is_json: db.store(f"users/{user}", { "password": generate_password_hash(request.get_json()['password'])})
else: db.store(f"users/{user}", { "password": generate_password_hash(request.form['password'])})

To fix that determine request data source beforehand. Optimizing register function:

@app.route('/regester', methods = ['POST'])
def register():
    data = request.get_json() if request.is_json else request.form
    user = data['username']
    if db.retrieve(f"users/{user}"):
        return False

    db.store(f"users/{user}", {"password": generate_password_hash(data['password'])})
    return make_response(user, 201)

Apply this optimization technique to all remaining functions with a similar issue.


Purely functional Sieve of Eratosthenes Many implementations of Sieve of Eratosthenes (used to find prime numbers up to a given n) use a temporary mutable array to keep track of which numbers are composites. I'm looking to write a purely functional version that has the same runtime complexity:

(define (primes-up-to n)
  (cons 2 (unfold null?
                  car
                  (compose remove-odd-multiples car+cdr)
                  (iota (quotient (- n 1) 2) 3 2))))

(define (remove-odd-multiples n xs)
  (let recur ((xs xs)
              (i (* n n)))
    (cond ((null? xs) '())
          ((< (car xs) i) (cons (car xs) (recur (cdr xs) i)))
          ((< i (car xs)) (recur xs (+ i n n)))
          (else (recur (cdr xs) (+ i n n))))))

(Should work on any Scheme implementation that supports SRFI 1. If your implementation doesn't provide compose, use (lambda (x) (remove-multiples (car x) (cdr x))) instead.)

What I'm hoping reviewers can help with: Is there a way to make remove-odd-multiples¹ less verbose (perhaps using higher order functions) or more efficient², without increasing the runtime complexity (so any kind of filter or remove involving non-constant-time predicates is no go)?

¹ Since xs does not contain even numbers, there's no point dropping even multiples, hence remove-odd-multiples. The only difference from a more general remove-multiples is that it advances i by 2n instead of n each time.

² I've done some casual timing tests on (primes-up-to 100000) on DrRacket 6.5 and the right-folding version you see above is faster than a left-folding version. (I mention this preemptively because many Scheme answerers on Stack Overflow lean towards left-folding solutions.) Here's the left-folding version I time-tested with:

(define (primes-up-to n)
  (reverse (unfold-right null?
                         car
                         (compose remove-odd-multiples car+cdr)
                         (iota (quotient (- n 1) 2) 3 2)
                         '(2))))

(define (remove-odd-multiples n xs)
  (let loop ((result '())
             (xs xs)
             (i (* n n)))
    (cond ((null? xs) (reverse result))
          ((< (car xs) i) (loop (cons (car xs) result) (cdr xs) i))
          ((< i (car xs)) (loop result xs (+ i n n)))
          (else (loop result (cdr xs) (+ i n n))))))

Here's a small refinement on the technique. Wherin instead of looking for odd multiples  and removing them, you keep an extra reference in the incomplete sieve and look those specific multiples of the next prime. The first step is the same, looking for 3* (3, 5, 7, 9, ...,) but the second step you only look for 5*(5, 7, 11, 13, 17, 19, 23 , 25, ...)

Overall a small speedup, but the upside is you will never be looking for a multiple that isn't either in the partial sieve or beyond it's range.

(define (primes-up2 n)
  (cons 2 (unfold null?
                  car
                  (lambda (x) (remove-partial-seive-multiples (car x) (cdr x)))
                  (iota (quotient (- n 1) 2) 3 2))))

(define (remove-partial-seive-multiples n partials)
  (let recur ((xs partials)
              (ys partials)
              (i (* n n)))
    (cond ((null? xs) '())
          ((< (car xs) i) (cons (car xs) (recur (cdr xs) ys i)))
          ((= (car xs) i) (recur (cdr xs) (cdr ys) (* (car ys) n)))
          (else (error "you ought not hit this")))))

This works as any multiple of n still in the partial sieve must also be coprime with all primes less than N. (of which the partial sieve is a complete list)

Now while not purely functional instead of keeping the result and reversing it in the end on your second iteration, you could implement it as a tail-modulo cons. While not stickly functional on the inside, it is functional from the outside.

Another optimization would be to pass the maximum canidate to the remover of multiples and just return the xs when i is smaller than the last search.


Modeling a classroom with teacher and students using the mediator pattern in Go I've made this implementation of a classroom with teacher and students using mediator pattern. Mediator is the teacher and colleague are studends.

Main function is the following. Main function creates a teacher and then she says will teach to some students. Finally, she try to spread a concept and a student will tell what he (or she) have learned.

func NewClassMate(name string) Student {
    return &ClassMate{name, "meeting 1", ""}
}

func main() {

    teacher := Teacher{}

    student := NewClassMate("Mario")

    teacher.TeachesTo(student)
    teacher.TeachesTo(NewClassMate("Demo"))
    teacher.TeachesTo(NewClassMate("Mattia"))
    teacher.TeachesTo(NewClassMate("Simone"))

    fmt.Println(len(teacher.attendees))
    fmt.Println(student.Forum())

    teacher.Spread("Message sent to everyone")

    fmt.Println(student.Learned())

}

Teacher

Teacher implementation is trivial. Teacher knows all students and spread lessons.

type Teacher struct {
    students []Student
}

func (m *Teacher) TeachesTo(c Student) {
    m.students = append(m.students, c)
}

func (m *Teacher) Spread(message string) {
    for _, a := range m.students {
        a.Learn(message)
    }
}

Student

Also student is very simple. It take part in a class, learns and say what he have learned.

type ClassMate struct {
    lastMessage string
    forum       string
}

func (a *ClassMate) Class() string {
    return a.forum
}

func (a *ClassMate) Learn(message string) {
    a.lastMessage = message
}

func (a *ClassMate) Learned() string {
    return a.lastMessage
}

Mediator and Colleagues

Finally here we can see interface I chosenm according to the pattern.

type Student interface {
    Class() string
    Learn(message string)
    Learned() string
}

type Mediator interface {
    TeachesTo(c *Student)
    Spread(messqger string)
}

Ok, let's start from the top: Is this a mediator pattern?

I'd argue what you have is 2 components of a mediator pattern (2 objects that interact) without an actual mediator. To just quote the first line of the wiki entry for the mediator pattern:

  In software engineering, the mediator pattern defines an object that encapsulates how a set of objects interact.

So it's an object encapsulating interactions between a set of objects. What you're doing is creating 2 types that interact, and because it's modelled on a teacher/student example, where students are quite passive, you're calling the teacher the mediator. However: how does the teacher encapsulate how a set of objects (ie the students) interact? It doesn't.

This isn't a bad thing, though, as you're probably coming from another, more classically OOP language, and you're trying to replicate what you're used to in go. That's a common mistake people make. Go is not a classical OO language, though. It doesn't have classes, yet has (ducktype/implicit) interfaces. It doesn't have inheritance, but has composition. It doesn't have exceptions, it doesn't have a specific things like static properties, or static methods. So please, don't force yourself to stick to the classical OO patterns. Use the freedom and expressive power Go brings through it's slightly different type system.

Other criticisms I have directly relating to your code:

func (a *ClassMate) Class() string {
   return a.forum
}

Why use a pointer receiver here? This is basically making what should be a perfectly safe function unsafe. Imagine:

go func() {
   student.forum = "some random message"
}()
fmt.Printf("student says: %s", student.Forum()) // or whatever

What is the output going to be? Can you be sure it's always going to be the same?

The same applies to the Learned() func for the student and Spread(messqger string) for the teacher/"Mediator". Especially the teacher, in fact. The Spread function might be fired off concurrently, as it may take some time to return. If you imagine a scenario where someone ends up doing something like this:

go teacher.Spread("some really important life-lesson")
teacher.TeachTo(anotherStudent)
// and some more calls like this
teacher.TeachTo(anotherStudent2)
teacher.TeachTo(anotherStudent3)

How confident are you that the result of the code above will be reliable and reproducible? I'd much rather have a copy of the slice head available to me at the point where the call is made, rather than to rely on the behaviour of a range loop. The moment you add some checks, or other logic in the Spread function, you're at risk of the behaviour changing.

Lastly, as noted in my comment: TeachesTo(c *Student) is invalid code. Student is an interface type, there is no pointer-to-interface type. It's either a pointer to a type implementing said interface, or the interface itself.

Moving on from the code itself, and more into more general comments/suggestions on how to approach things in a more idiomatic golang type of way.

I can't help noticing you're calling this an awful lot in succession:

teacher.TeachesTo(student)

Golang supports for variadic arguments, just pass in all the students at once. The name of the func is also quite misleading. If I see TeachesTo, I expect the call to actually interact with the object I'm passing in, as and when I pass it in. I don't expect a later call to Spread to actually perform the action. I'm expecting to be able to call TeachesTo, and be done with it. I'd change the Mediator interface to a Classroom or Course interface along the lines of:

type Course struct {
    EnterTeacher(t Teacher) error // Teacher being an interface
    EnterStudents(s ...Student) error // variadic, only requires a single call
    AddLesson(data []string) error // using []string, but probably should be structured
    StartLesson() error
    TeacherLeave() error
    StudentsLeave() error
    EndLesson() error
    GetComplaints() []string // complain type?
    GetBannedStudents() []Student
    GetScores() map[string]float64 // results by student name?
    // and so on...
}

Now we actually have a mediator. We just add a teacher, add one or more lessons, we can start/stop lessons, get some kind of state (like scores, banned students, complaints about the way the teacher interacts with the students etc...

The implementation could be a simple thing like:

type course struct {
    name       string
    t          Teacher
    s          []Student
    lessons    []Lesson // create a type that's fitting
    day        int // can be used as offset in lessons
    scores     map[string]float64
    complaints []string
    banned     map[string]Student
}

Other repercussions of this approach: you'll notice both the type and the fields are not exported. That's how it should be: the whole point of a mediator is that you encapsulate the way objects interact. You shouldn't leave them exposed to external interaction, for that very reason.

Usage, quite simple

package school // or whatever

// google functional options if this is confusing you
type CourseOption func (c *course)

// add funcs like these for banned students, lessons, students, etc...
func SetCourseTeacher(t Teacher) CourseOption {
    return func(c *Course) {
        c.t = t
    }
}

func NewCourse(n string, opts ...CourseOption) Course {
    c := &course{
        name: n,
        scores: map[string]float64{},
        banned: map[string]Student{}, // initialise these to avoid issues when accessing these
    }
    for _, o := range opts {
        o(c)
    }
    return c
}
// a couple of implementation examples:

func (c *course) EnterStudents(s ...Student) error {
    // ensure we're not entering a banned student
    for i := range s {
        if _, ok := c.banned[s[i].Name()]; ok {
            return fmt.Errorf("student %s is banned", s[i].Name())
        }
        if _, ok := c.scores[s[i].Name()]; ok {
            return fmt.Errorf("student %s already entered", s[i].Name())
        }
    }
    // this is messy, but required
    for i := range s {
        s.scores[s[i].Name()] = 0 // add new name to score list
    }
    c.s = append(c.s, s...) // append students from arg to students in list
    return nil
}

As you can see, I can now safely add any number of students, and return errors in case some of them are banned already, and a different error in case the student was already added. It also initialises the state correctly. Now let's see some actual mediation implemented:

func (c *course) StartLesson() error {
    if len(c.lessons) <= c.day {
        return fmt.Errorf("no lesson can be started, Last lesson %d of %d", c.day, len(c.lessons))
    }

    // mediator decides which lesson object the teacher uses, and determines the teacher should prepare it
    if err := c.t.PrepareLesson(c.lesson[c.day]); err != nil {
        return err
    }
    // pass in students to which to teach the prepared lessen to, like a mediator does
    if err := c.t.Teach(c.s...); err != nil {
        return err
    }
    return nil
}

func (c *course) EndLesson() error {
    if !c.t.InClass() {
        // obviously...
        return fmt.Errorf("teacher not in class, cannot end lesson")
    }
    // see if teacher banned anyone
    for _, banned := range c.t.ProblemStudents() {
        // find student in slice, cut from slice and add to banned map
        // at this point, a map of students is easier to handle, but I can't be bothered
        // to rewrite most of this answer ;-p
    }
    for i := range c.s {
        // listen to complaints from students, too
        c.complaints = append(c.complaints, c.s[i].Complaints()...)
    }
    // mediator checks if the lesson was important, and instructs the teacher
    // to prepare a test. How the test is taken, is part of the lesson preparation and the teaching implementations
    if c.lessons[c.day].Important() {
        c.t.PrepareTest()
    }
    c.day++ // increment this, so we don't accidentally start the same lesson again
    return nil
}

Now at this point, this is getting rather convoluted and complex. It's suffering from the classical OEP-bloat (Over-Engineered-Programming). Something the golang designers wanted to discourage in their language.

Note that all of the code I provided above is very unsafe the moment you're going to use it with concurrency: the use of maps is, by definition unsafe. If you have someone calling:

go course.EnterStudents(studentSlice[4:]...)
go course.EnterStudents(studentSlice...)
studentSlice := append([]Student{prependStudent}, studentSlice[4:6]...)
go course.EnterStudents(studentSlice...)

Guess what will happen, that is: if it's even possible... (hint: it's not).

I'd recommend you use the sync package, and always test/compile with the race-detector.


Truncate 160-bit output from SHA-1 to 64-bit uint64_t I'd like to truncate 160-bit output from SHA-1 to receive a (weaker) 64-bit digest.

It has been a while since I did the type of low-level C pointer arithmetic in the but-last line. Could you please review it for me? It should return the last 64 bits of the the byte array in a uint64_t. (I guess conceptually I could just as well take the first 64 bits, which would make the code simpler but here we are.)

#import <CommonCrypto/CommonDigest.h>

- (uint64_t)digestFromString: (NSString *)s {
    NSData *input = [s dataUsingEncoding: NSUTF8StringEncoding];
    unsigned char output[CC_SHA1_DIGEST_LENGTH];
    CC_SHA1(input.bytes, (CC_LONG)input.length, output);
    uint64_t digest;
    NSAssert(CC_SHA1_DIGEST_LENGTH >= sizeof(uint64_t), nil);
    digest = *(uint64_t *)(output + CC_SHA1_DIGEST_LENGTH - sizeof(uint64_t));
    return digest;
}

How could this code be improved upon?

UPDATE Here is a second (alternative) version with an attempt at avoiding any alignment issues. (Its much more straigtforward to truncate to the first 64 bits in this case.) Actual digests will differ from the 1st version.

- (uint64_t)digestFromString: (NSString *)s {
    NSData *input = [s dataUsingEncoding: NSUTF8StringEncoding];
    union { // uint64_t-aligned
        uint64_t      uint64s[CC_SHA1_DIGEST_LENGTH / sizeof(uint64_t) + sizeof(uint64_t)];
        unsigned char bytes[CC_SHA1_DIGEST_LENGTH];
    } output;
    CC_SHA1(input.bytes, (CC_LONG)input.length, (unsigned char *)output.bytes);
    return output.uint64s[0];
}

Both your methods should work. As far as I know, there is no alignment problem
on the current OS X and iOS architectures in your first method (see below).

The code from the second method can be simplified a tiny bit:

- (uint64_t)digestFromString: (NSString *)s {
    NSData *input = [s dataUsingEncoding: NSUTF8StringEncoding];
    union { // uint64_t-aligned
        uint64_t      digest;
        unsigned char bytes[CC_SHA1_DIGEST_LENGTH];
    } output;
    CC_SHA1(input.bytes, (CC_LONG)input.length, output.bytes);
    return output.digest;
}

The first element in the union need not be an array, and casting output.bytes
to unsigned char * is not necessary (the array automatically becomes a pointer
to the first element when passed as a function argument).

But the easiest method to extract a 64-bit integer from the digest would be to use the inline function

OS_INLINE uint64_t _OSReadInt64(
    const volatile void               * base,
    uintptr_t                     byteOffset
)

from OSByteOrder.h that comes with the OS X and iOS SDKs:

uint64_t digest = _OSReadInt64(output, CC_SHA1_DIGEST_LENGTH - sizeof(uint64_t)); // last 8 bytes
uint64_t digest = _OSReadInt64(output, 0); // first 8 bytes

This function is implemented as

return *(volatile uint64_t *)((uintptr_t)base + byteOffset);

which indicates that there is no alignment problem when casting an
arbitrary pointer to uint64_t *. But that may be different on future
architectures, with using the above function you are on the safe side.

Alternatively, you can use memcpy:

uint64_t digest;
memcpy(&digest, output, sizeof digest);

Instead of

NSAssert(CC_SHA1_DIGEST_LENGTH >= sizeof(uint64_t), nil);

you could use a "compile-time/static assertion":

_Static_assert(CC_SHA1_DIGEST_LENGTH >= sizeof(uint64_t), "");

_Static_assert is a GCC extension which is also understood by the Clang
compiler, so you can use it in your Xcode projects.

The advantage is that it causes a compiler error if the condition is
not satisfied, instead of failing later at runtime.


Script for saving/sorting/sending emails based on their subject I've spent a couple days making this script in Outlook that checks my emails and saves/sorts/sends a reply based on the subject. This also happens to be my first experience in working with VBA. Now that I have it working, I was wondering how I could simplify it or "make it prettier" (I always got docked points in my C++ class for having "ugly code").

DOC and PDF are unrelated to .doc and .pdf. They're just my stupid renaming of what are actually two different types of names for .pdf files.

Public Sub saveattachtodisk(itm As Outlook.MailItem)
Dim objAtt As Outlook.Attachment
Dim saveFolder As String
Dim saveName As String
Dim whatIs As String
Dim emailAttach As String
Dim attName As String
saveName = itm.Subject
whatIs = retArray(saveName)
saveFolder = "C:\Users\USER\Desktop\Staging"
For Each objAtt In itm.Attachments
    If whatIs = "PDF" Then
        objAtt.SaveAsFile saveFolder & "\" & UCase(saveName) & ".pdf"
ElseIf whatIs = "DOC" Then
    objAtt.SaveAsFile saveFolder & "\" & "DOC 14-" & stripNums(saveName) & ".pdf"
    emailAttach = saveFolder + "\" + "DOC 14-" + stripNums(saveName) + ".pdf"
    attName = stripNums(saveName)
    sendDOC emailAttach, attName
ElseIf whatIs = "other" Then
    objAtt.SaveAsFile saveFolder & "\" & saveName & ".pdf"
End If
objAtt = Nothing
Next
End Sub

'Pulls numbers from DOC type document
Function stripNums(s As String) As String
Dim saveName As String
Dim i As Integer
For i = 1 To Len(s)
    If Mid(s, i, 1) >= "0" And Mid(s, i, 1) <= "9" Then
        saveName = saveName + Mid(s, i, 1)
    End If
Next
stripNums = saveName
End Function

'Checks to see if document is PDF type docement
Function isPDF(s As String) As Boolean
Dim retVal As Boolean
retVal = False
For i = 1 To Len(s)
If Mid(s, i, 2) = "1f" Then
    retVal = True
End If
Next
isPDF = retVal
End Function

'Checks to see if document is of type PDF, DOC, or other
Function retArray(s As String) As String
Dim retVal As String
Dim isItPDF As Boolean
retVal = "other"
arr = Array("t 1", "t 2", "t 3", "t 4", "t 5", "t 6", "t 7", "t 8", "t 9", "t 0")
isItPDF = isPDF(s)
If isItPDF = False Then
For j = 0 To 9
    If InStr(1, s, arr(j), 1) = 1 Then
        retVal = "DOC"
    End If
Next
ElseIf isItPDF = True Then
    retVal = "PDF"
End If
    retArray = retVal
End Function

'Automatically creates and sends an email for DOC type documents
Function sendDOC(s As String, n As String)
Dim objMsg As MailItem
Set objMsg = Application.CreateItem(olMailItem)
objMsg.Recipients.Add ("SEND.TO@EMAIL.COM")
objMsg.Recipients.Add ("DOUBLE.CHECK@EMAIL.COM")
objMsg.Subject = "DOC 14-" & n
objMsg.Body = "Please see attached." & vbCrLf & vbCrLf & vbCrLf & "SIGNATURE BLOCK"
objMsg.Attachments.Add (s)
objMsg.Send
End Function

Readability

Proper indentation is important for readability. Everything inside of Sub...End Sub should be indented one tab (or four spaces). Else If statements should line up with their corresponding If statements. For example:

Public Sub foo
    dim variable as boolean
    If variable Then
        'do something
    Else
        'do something different
    End If
End Sub

Variable and routine names are overly abbreviated. We're no longer in the bad ol' days when the length of variable names were limited. Instead of attName, try attachmentName. Apply this to all of your variable names and Mr. Maintainer will thank you.
As a matter of opinion, Sub and Function names should be PascalCase. Instead of saveattachtodisk, try SaveAttachmentToDisk. I do like that you're following the verb-noun naming schema for them though. You did a good job there.
Back to over abbreviation. There are single letter parameter names. They tell me nothing about what I'm supposed to pass into them.

Functionality

What happens if "C:\Users\USER\Desktop\Staging" doesn't exist on the client machine? The code crashes. Adding some error handling in will help, but the routine should also either let the user pick a folder, or build the directory itself. It's looks like it's a temporary structure, so I recommend the latter.
Let's talk about this section for a minute.

If whatIs = "PDF" Then
    objAtt.SaveAsFile saveFolder & "\" & UCase(saveName) & ".pdf"
ElseIf whatIs = "DOC" Then
    objAtt.SaveAsFile saveFolder & "\" & "DOC 14-" & stripNums(saveName) & ".pdf"
    emailAttach = saveFolder + "\" + "DOC 14-" + stripNums(saveName) + ".pdf"
    attName = stripNums(saveName)
    sendDOC emailAttach, attName
ElseIf whatIs = "other" Then
    objAtt.SaveAsFile saveFolder & "\" & saveName & ".pdf"
End If

This works great for now, but what happens when I want to expand it to include other document types? I'm thinking a DocType enum could go a long way toward making it easier to extend, as well as improve readability.

Public Enum DocType
    PDF
    DOC
End Enum

Select Case whatIs
    Case PDF
        objAtt.SaveAsFile saveFolder & "\" & UCase(saveName) & ".pdf"
    Case DOC
        objAtt.SaveAsFile saveFolder & "\" & "DOC 14-" & stripNums(saveName) & ".pdf"
        emailAttach = saveFolder + "\" + "DOC 14-" + stripNums(saveName) + ".pdf"
        attName = stripNums(saveName)
        sendDOC emailAttach, attName
    Case Else
        objAtt.SaveAsFile saveFolder & "\" & saveName & ".pdf"
End Case

The "Doc 14-" string show up a lot. Store it in a constant at the module scope.
saveFolder should be a constant at the procedure scope. If it doesn't change, declare it as a constant so Mr. Maintainer doesn't try to assign it a new value at runtime in the future.
You can break out of the for loop in isPDF() as soon as it finds the value it's looking for. There's also no reason for the returnValue variable to exist. Just assign the function value directly. Booleans are implicitly false upon creation, so isPDF() could look more like this.

'Checks to see if document is PDF type docement
Function isPDF(s As String) As Boolean
    For i = 1 To Len(s)
        If Mid(s, i, 2) = "1f" Then
            isPDF = True
            Exit For
        End If
    Next
End Function

There are probably a couple of other things that could be said, but I'll leave those to another reviewer.


Are private extension methods a bad thing to use? In C# I have the following two extension methods.

    public static void WaitForMilliseconds(this IWebDriver driver, int milliseconds)
    {
        var timeout = new TimeSpan(0, 0, 0, 0, milliseconds);
        WaitForTimeout(driver, timeout);
    }

    public static void WaitForSeconds(this IWebDriver driver, int seconds)
    {
        var timeout = new TimeSpan(0, 0, 0, seconds);
        WaitForTimeout(driver, timeout);
    }

    private static void WaitForTimeout(IWebDriver driver, TimeSpan timeout)
    {
        try
        {
            new WebDriverWait(driver, timeout).Until(x => false);
        }
        catch (Exception){}
    }

But since the class is static, I can write a private extension method to make the syntax more readable like this.

    public static void WaitForMilliseconds(this IWebDriver driver, int milliseconds)
    {
        var timeout = new TimeSpan(0, 0, 0, 0, milliseconds);
        driver.WaitForTimeout(timeout);
    }

    public static void WaitForSeconds(this IWebDriver driver, int seconds)
    {
        var timeout = new TimeSpan(0, 0, 0, seconds);
        driver.WaitForTimeout(timeout);
    }

    private static void WaitForTimeout(this IWebDriver driver, TimeSpan timeout)
    {
        try
        {
            new WebDriverWait(driver, timeout).Until(x => false);
        }
        catch (Exception){}
    }

Is there any inherently bad code smell about private extension methods? Which would is better to use? The only issue I see is that if the code is later refactored into a class the methods will have to be rewritten.

In my opinion, extension methods are only a (pretty nice) syntactic sugar for a static helper method. Under the hood, the IL translates your extension method call to a call on the static method, so really it's just an other way to write methods.

The extension method do not have a private access to the type they are extending either, so changing a static method into an extension one will not break any encapsulation.

As for the private keyword, it is indeed the correct accessibility level since your method will not be used outside of your class.

According to MSDN's Guidelines, the major problem of extension methods is the following :

  When using an extension method to extend a type whose source code you cannot change, you run the risk that a change in the implementation of the type will cause your extension method to break.

Also, if someday IWebDriver defines a new method called "WaitForTimeout", your extension method won't be called anymore... But the risk is really pretty low.

So if you think readability is increased by using that extension method, use it - you won't violate any principles or break anything, see it as a nice syntactic sugar.