C Primer Plus - Chapter 4 - Task 6 (string output) Task

Write a Program that requests the user's first name and then the user's last name. Have it print the entered names on one line and the number of letters in each name on the following line. Align each letter count with the end of the corresponding name, as in the following:

Klaus Dieter
    5      6

My Try

#include <stdio.h>
#include <string.h>

void printWhitespace(int times) {
    for (int i = 0; i < times; i++) {
        printf(" ");
    }
}

int main(void) {
    // get names
    printf("first name: ");
    char firstName[20];
    scanf("%s", &firstName);
    printf("last name: ");
    char lastName[20];
    scanf("%s", &lastName);

    // display names
    printf("%s %s\n", firstName, lastName);

    // display number of characters under last character of names
    int numOfWhitespace = strlen(firstName) - 1;
    printWhitespace(numOfWhitespace);
    printf("%d ", strlen(firstName));

    numOfWhitespace = strlen(lastName) - 1;
    printWhitespace(numOfWhitespace);
    printf("%d\n", strlen(lastName));
}

Does the author expect that from me? Or is there a better way to write that?

Small sugggestion for printWhitespace(). You could do the following:

void printWhitespace(const unsigned int times) {
    printf("%*s", times, " ");
}

I have made the function argument const as it probably isn't meant to be modified. it is generally a good idea to be as "const" as possible as this avoids the mistake of writing to a variable that should be read-only.

I would put the array declarations at the top of the function before any of the code with a blank line between the declarations and the first line of code.

The function scanf can be used with caution... it can lead to buffer overflow attacks in the way it is used in your code. If the user enters a string longer than 19 characters (last character would be filled in as a null terminator in the buffer), scanf will just write on past the end of the buffer.

You could help guard against this by using scanf("%19s", firstName); and scanf("%19s", lastName);. As @chux points out the length is one less than the buffer size. This is because "String input conversions store a terminating null byte ('\0') to  mark end of the input; the maximum field width does not include this terminator." -- quote from man page.

Looked into this a little more and this SO answer, the author says:

  Note that the POSIX 2008 (2013) version of the scanf() family of
  functions supports a format modifier m (an assignment-allocation
  character) for string inputs (%s, %c, %[). Instead of taking a char *
  argument, it takes a char ** argument, and it allocates the necessary
  space for the value it reads

That would be a useful way of avoiding buffer overflow, but you must remember to free() the buffer returned.

The variable numOfWhitespace can also be const. Might put that to top of function too.

Your last bit of code that tries to align the numbers to the end of the words will only align properly if the string length is 9 or less. If the string length is greater then the number will be double digits so you could account for this.

Add return 0; to the end of the function. You reach the end of a non-void function without returning anything...

Hope that helps :)


Lexer Implementation in Rust - `PhantomData` Awkwardness I've been working on a Tokeniser/Lexer in Rust. I would like to have the tokeniser take input from different sources, such as files or in-memory strings. To abstract over this concern i've created a trait Source. This has left me with a wonky situation where I appear to need a PhantomData member in the token iterator. I've created a cut-down example here:

/// Source of Characters
///
/// In this example all that a source can do is be sliced to retrieve
/// a subsection of the overall character buffer.
trait Source<'a> {
    /// Character at Offset
    ///
    /// Gets the character at the given offset in the buffer and
    /// returns it. If no character is available at that offset `None`
    /// is returned.
    fn at(&self, offset: usize) -> Option<(char, usize)>;

    /// Slice the Source Buffer
    fn slice(&self, start: usize, end: usize) -> &'a str;
}

/// Source of Characters from a `str` Slice
struct StringSource<'a> {
    pub buff: &'a str
}

/// Implementation of the Source trait.
impl<'a> Source<'a> for StringSource<'a> {
    fn at(&self, offset: usize) -> Option<(char, usize)> {
        self.buff[offset..].chars().nth(0).map(|ch| { (ch, offset + ch.len_utf8()) })
    }

    fn slice(&self, start: usize, end: usize) -> &'a str {
        &self.buff[start..end]
    }
}

/// Token Iterator Implementation
///
/// This token iterator takes a given source and steps through it returning string slices for each token
struct TokenIter<'a, S>
    where S: Source<'a>,
          S: 'a
{
    source: S,
    idx: usize,
    phantom: ::std::marker::PhantomData<&'a ()>,
}

impl<'a, S> TokenIter<'a, S>
    where S: Source<'a>
{
    /// Create a Token Iterator from a Source
    fn new(source: S) -> Self {
        TokenIter {
            source: source,
            idx: 0,
            phantom: ::std::marker::PhantomData,
        }
    }
}

/// Token Iterators implement `Iterator`
impl<'a, S> Iterator for TokenIter<'a, S>
    where S: Source<'a>
{
    type Item = &'a str;

    fn next(&mut self) -> Option<Self::Item> {
        let ts = self.idx;
        self.source.at(ts).map(|(_ch, next)| {
            self.idx = next;
            // Imagine a regex state machine is run here to produce a token
            // rather than just returning single-character tokens.
            self.source.slice(ts, next)
        })
    }
}

fn main() {
    let source = StringSource{ buff: "hello world" };
    let iter = TokenIter::new(source);
    println!("{:?}", iter.collect::<Vec<_>>());
}

Run it here:
https://is.gd/OCL91u

Is there a nicer way to express the source buffer lifetime 'a so that I don't need a PhantomData member or this bit of funky lifetime constraints:

where S: Source<'a>,
      S: 'a

such as files or in-memory strings

I'm going to give the advice that people dread: I don't think your abstraction is going to work here. When I first read "in-memory strings", I expected a String, not a &str. Since you mentioned a file, I think it's still a valid comparison. I don't believe you can implement this trait for such a type:

struct OwnedStringSource {
    pub buff: String,
}

impl<'a> Source<'a> for OwnedStringSource {
    fn at(&self, offset: usize) -> Option<(char, usize)> { None }

    fn slice(&self, start: usize, end: usize) -> &'a str {
        // Hmm.... what to put here?
    }
}

That is, there's no way to say a reasonable lifetime for 'a. I also think that it's the same root problem as Can I write an Iterator that yields a reference into itself?.

Aside from that, your original error was probably something like:

error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates
   --> src/main.rs:117:6
    |
117 | impl<'a, S> Iterator for TokenIter<S>
    |      ^^ unconstrained lifetime parameter

The best advice I've gotten about that specific error was helpful... after thinking about it for a while. Paraphrased and elided for this situation:

  what [the error is] trying to tell you is that it cannot get [the generic type] back from either
  the implemented trait [...] or the type implemented on [...]. [The where clause]
  is not enough to extract [the generic] from [the type] because one [...] type can have multiple
  [...] impls with various arguments

Instead, I might suggest that you use an associated type instead of the generic parameter:

trait Source {
    type Slice;

    fn at(&self, offset: usize) -> Option<(char, usize)>;
    fn slice(&self, start: usize, end: usize) -> Self::Slice;
}

This separates the lifetimes from the trait. Specific implementations can still participate in it:

impl<'a> Source for StringSource<'a> {
    type Slice = &'a str;
    // ...
}

And you can just bubble up the inner type out of the iterator:

impl<'a, S> Iterator for TokenIter<S>
    where S: Source,
{
    type Item = S::Slice;
    // ...
}

You potentially might need to add extra bounds on that generic there (S::Slice: AsRef<str>) depending on what you need to be able to do with the slice in the iterator implementation.

trait Source {
    type Slice;

    fn at(&self, offset: usize) -> Option<(char, usize)>;
    fn slice(&self, start: usize, end: usize) -> Self::Slice;
}

struct StringSource<'a> {
    pub buff: &'a str
}

impl<'a> Source for StringSource<'a> {
    type Slice = &'a str;

    fn at(&self, offset: usize) -> Option<(char, usize)> {
        self.buff[offset..].chars().nth(0).map(|ch| { (ch, offset + ch.len_utf8()) })
    }

    fn slice(&self, start: usize, end: usize) -> &'a str {
        &self.buff[start..end]
    }
}

struct TokenIter<S> {
    source: S,
    idx: usize,
}

impl<S> TokenIter<S> {
    fn new(source: S) -> Self {
        TokenIter {
            source: source,
            idx: 0,
        }
    }
}

impl<S> Iterator for TokenIter<S>
    where S: Source,
{
    type Item = S::Slice;

    fn next(&mut self) -> Option<Self::Item> {
        let ts = self.idx;
        self.source.at(ts).map(|(_ch, next)| {
            self.idx = next;
            self.source.slice(ts, next)
        })
    }
}

fn main() {
    let source = StringSource{ buff: "hello world" };
    let iter = TokenIter::new(source);
    println!("{:?}", iter.collect::<Vec<_>>());
}


Similar methods using loops Is it recommended to combine two similar methods into one?

public void initialize_board(){
    for(int y = 0; y < board.length; y++){
        for(int x = 0; x < board.length; x++){
            if(x == 0)
                board[x][y] = (char)y;
            else
                board[x][y] = '~';
        }
    }
}

public void display_board(){
    for(int y = 0; y < board.length; y++){
        for(int x = 0; x < board.length; x++){
            System.out.print(board[x][y]);
        }
        System.out.print("\n");
    }
}

Not so similar...

Just because the method bodies "look similar" doesn't make them similar. It's better to think in terms of purpose instead. The two methods are for completely different things. They are not similar, and good to be separated.

In functional languages you could use a "walk" method to iterate over all cells of the board and use a callback function to act on each cell. You could emulate that behavior in Java, but the code will get more complex, and I'm not sure it will be worth it in this example.

Robustness

Do you intend to initialize the board more than once in the lifetime of the object? If not (probably), then move the initialization to the constructor.

Writing style

You should use camelCase for naming, for example initializeBoard instead of initialize_board.

It's recommended to use braces with ifs, for example:

if (x == 0) {
    board[x][y] = (char)y;
} else {
    board[x][y] = '~';
}

It's better to print a newline using println:

System.out.println();
// instead of: System.out.print("\n");


Specifying UTC time or current time offset What do you think about this helper class to specify UTC time? It should also support an easy way to capture current time offsets. Let's say we expose it in API like this:

interface IApi
{
    void Start(Utc at);
}

Now it should go as:

api.Start(new DateTime(2021, 01, 01)) // starts on 2021-01-01 UTC
api.Start(new TimeSpan(1, 0, 0)) // starts in one hour
api.Start(2000) // starts in 2 seconds

Library code is:

struct Utc
{
    public static Utc Now => 0;

    public static implicit operator Utc(DateTime value) => new Utc(value);
    public static implicit operator Utc(DateTimeOffset value) => value.UtcDateTime;
    public static implicit operator Utc(TimeSpan value) => DateTime.UtcNow + value;
    public static implicit operator Utc(int value) => TimeSpan.FromMilliseconds(value);

    public static implicit operator DateTime(Utc utc) => utc.Time;
    public static implicit operator DateTimeOffset(Utc utc) =>
        new DateTimeOffset(utc, TimeSpan.Zero);

    Utc(DateTime time) => Time = DateTime.SpecifyKind(time, DateTimeKind.Utc);
    DateTime Time { get; }
}

In terms of conventions there is nothing to poke at there.

Where I find that approach lacking is that it tries to merge two different concepts. Namely a point in time an a time difference (or offset). Another non .NET example that I can think of is swift which has Date and TimeInterval. TimeInterval being an alias for a double which represents the offset in seconds.

Most libraries (.NET included) have distinct types to separate these two concepts.

To illustrate where it goes wrong I would expect

Utc.Now == DateTime.Now

But it is not.

I understand the convenience for have the common type and using the type system to enforce this for you but consumers of the API would have some trouble with it.

Two alternatives:

Use a built in library type. Either DateTime to specify the exact time of execution or TimeSpan.

In this case DateTime provides the utility of ToUniversalTime() but that would require the implementer of the interface IApi to ensure that this is done. As long as you clearly document what you expect it is OK.

An added benefit is that you don't have to reinvent the wheel and your code is now more portable between all .NET platforms. These types include IComparable and IEquatable already which is one less thing to worry about for the future.

Keep the type Utc but make sure you keep the underlying type consistent to either by an offset or a fixed time.

Either one works. I would lean to storing the backing data as TimeSpan. Converting a time to the time interval from when it was called.

The implicit conversions in this case are a good idea.


Using as few queries as possible for many table associations I have a table containing about 30k rows of data and there's several table associations. The problem is that there's like 18 columns, and each column requires 1 db query.

Here's an example of what I'm doing:

if col_num == 0
    tmp_string_text = finding_phase
elsif col_num == 1
    tmp_string_text = ""
elsif col_num == 2
    tmp_string_text = ""
elsif col_num == 3
    tmp_string_text =  finding.vuln_finding.severity
elsif col_num == 4
    tmp_string_text = finding.node.ip
elsif col_num == 5
    tmp_string_text =  finding.node.host_name
elsif col_num == 6
    tmp_string_text =  finding.node.dns_name
elsif col_num == 7
    tmp_string_text = finding.port
elsif col_num == 8
    tmp_string_text =  finding.pentest_finding.name
elsif col_num == 9
    tmp_string_text =  finding.vuln_finding.name
elsif col_num == 10
    tmp_string_text = finding.vuln_finding.description
elsif col_num == 11
    tmp_string_text = finding.vuln_finding.solution
elsif col_num == 12
    tmp_string_text = finding.additional_output
elsif col_num == 13
    tmp_string_text = finding.cve
elsif col_num == 14
    tmp_string_text = finding.node.os
elsif col_num == 15
    tmp_string_text =  finding.node.device_type
elsif col_num == 16
    tmp_string_text =  finding.node.scan_time
end

As a result, there's going to be 15 queries to the DB per 1 row. Is there any way that I can make this more efficient? This takes a REALLY long time when I have 30k rows. Not looking for anyone to flame here, just looking for advice. Pretty new to Ruby on Rails but learning as I go.

Basically, "finding" in this case is a result of @my_report.findings and it's going through each finding and pulling back data.

When I see long lists of if/elsif, I look to see whether case/when is a good alternate:

tmp_string_text = case col_num
                  when 0
                    finding_phase
                  when 1, 2
                    ""
                  when 3
                    finding.vuln_finding.severity
                  when 4
                    finding.node.ip
                  when 5
                    finding.node.host_name
                  when 6
                    finding.node.dns_name
                  when 7
                    finding.port
                  when 8
                    finding.pentest_finding.name
                  when 9
                    finding.vuln_finding.name
                  when 10
                    finding.vuln_finding.description
                  when 11
                    finding.vuln_finding.solution
                  when 12
                    finding.additional_output
                  when 13
                    finding.cve
                  when 14
                    finding.node.os
                  when 15
                    finding.node.device_type
                  when 16
                    finding.node.scan_time
                  end

In this case I think it's a reasonable solution. If you were only returning strings then a hash would be even more compact:

tmp_string_text = {
  0 => 'finding_phase',
  1 => "",
  2 => "",
  3 => 'finding.vuln_finding.severity',
}[col_num]

That's obviously truncated, but you get the idea.

But, again, you're executing code for your return value once your condition matches. Trying to use a hash will cause all that code to execute when the hash is initialized, which is likely to not be what you want.

As an exercise, I tried clarifying what's going on by using nested case statements:

tmp_string_text = case col_num
                  when 0
                    finding_phase
                  when 1 .. 2
                    ""
                  when 3
                    finding.vuln_finding.severity
                  when 4 .. 6
                    meth = case col_num
                          when 4
                            :ip
                          when 5
                            :host_name
                          when 6
                            :dns_name
                          end
                    finding.node.send(meth)
                  when 7
                    finding.port
                  when 8
                    finding.pentest_finding.name
                  when 9 .. 11
                    meth = case col_num
                          when 9
                            :name
                          when 10
                            :description
                          when 11
                            :solution
                          end
                    finding.vuln_finding.send(meth)
                  when 12
                    finding.additional_output
                  when 13
                    finding.cve
                  when 14 .. 16
                    meth = case col_num
                          when 14
                            :os
                          when 15
                            :device_type
                          when 16
                            :scan_time
                          end
                    finding.node.send(meth)
                  end

Obviously that's untested. It's a bit longer, but the nesting does help reveal  the logic.

Then, as a DRY exercise, took that and tried to reduce the repetition a bit:

case col_num
when 0
  finding_phase
when 1 .. 2
  ""
when 3 .. 16

  result = finding

  methods = case col_num
            when 3
              [:vuln_finding, :severity]
            when 4 .. 6
              [:node] << case col_num
              when 4
                :ip
              when 5
                :host_name
              when 6
                :dns_name
              end
            when 7
              [:port]
            when 8
              [:pentest_finding, :name]
            when 9 .. 11
              [:vuln_finding] << case col_num
              when 9
                :name
              when 10
                :description
              when 11
                :solution
              end
            when 12
              [:additional_output]
            when 13
              [:cve]
            when 14 .. 16
              [:node] << case col_num
              when 14
                :os
              when 15
                :device_type
              when 16
                :scan_time
              end
            end
  methods.each do |m|
    result = result.send(m)
  end
  result
end

Again that's not tested. It loses some readability at that point but it's pretty easy to figure out what's going on.


Fizz Buzz Bizz Fuzz in Java This questions is originally from http://contestcoding.wordpress.com/2013/06/28/fizz-buzz-bizz-fuzz/.

  Print the integers from 1 to 100,
  but for the multiples of 3, print "Fizz" instead and
  for multiples of 5, print "Buzz".
  If the number contains a 3 (for example 23), print "Bizz" and
  if the number contains a 5, print “Fuzz”
  (if it contains multiple 3s or 5s, just print one "Bizz" or "Fuzz").
  If the number contains more than one of these attributes, print every word (for example 33 prints "FizzBizz", as 33 is both a multiple
  of 3 and contains the digit 3).

My Java solution is:

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i < 101; i++) {
            // Set this to true when one of the special conditions is met.
            boolean printed = false;

            if (i % 3 == 0) {
                // When i is divisible by 3, then print "Fizz"
                printed = true;
                System.out.print("Fizz");
            } else if (i % 5 == 0) {
                // When i is not divisible by 3 but is divisible by 5, then print "Buzz"
                printed = true;
                System.out.print("Buzz");
            }

            if (Integer.valueOf(i).toString().indexOf("3") != -1) {
                // When i has the digit 3 in it, then print "Bizz"
                printed = true;
                System.out.print("Bizz");
            } else if (Integer.valueOf(i).toString().indexOf("5") != -1) {
                // When i has the digit 5 in it, then print "Fuzz"
                printed = true;
                System.out.print("Fuzz");
            }

            if (printed == false) {
                // The number does not satisfy any of the special conditions above.
                System.out.print(i);
            }
            System.out.println();
        }
    }
}

Please provide code review comments.

Overall, this is a pretty straightforward program. See the bottom of my answer for an Extreme Makeover: Code Edition of the program.

You have a hard-coded "magic number" in your for loop.  It would be better to use a variable.

for (int i = 1; i < 101; i++) // not the best

int num = 101;
for (int i = 1; i < num; i++); // better

Your if test conditions can be shortened a bit.

if (Integer.toString(i).indexOf("3") != -1)

Your logic is a bit off.  You should actually have fewer else if conditions (this is rarely the case, but here it is applicable).  For example, when i reaches "15", is should print "FizzBuzzFuzz", but your program only prints "FizzFuzz".

Final code:

public class Test
{
    public static void main(String... args)
    {
        int num = 101;
        for (int i = 1; i < num; i++)
        {
            boolean printed = false;

            if (i % 3 == 0)
            {
                printed = true;
                System.out.print("Fizz");
            }
            if (i % 5 == 0)
            {
                printed = true;
                System.out.print("Buzz");
            }

            if (Integer.toString(i).indexOf("3") != -1)
            {
                printed = true;
                System.out.print("Bizz");
            }
            if (Integer.toString(i).indexOf("5") != -1)
            {
                printed = true;
                System.out.print("Fuzz");
            }

            if (printed == false) System.out.print(i);
            System.out.println();
        }
    }
}

Extreme Makeover: Code Edition

Let's use a StringBuilder and some ternary operators.  And let's get rid of that boolean.

public class Test
{
    public static void main(String... args)
    {
        int num = 101;
        for (int i = 1; i < num; i++)
        {
            StringBuilder sb = new StringBuilder();
            if(i % 3 == 0) sb.append("Fizz");
            if(i % 5 == 0) sb.append("Buzz");

            if(Integer.toString(i).indexOf("3") != -1) sb.append("Bizz");
            if(Integer.toString(i).indexOf("5") != -1) sb.append("Fuzz");

            if (sb.length() == 0) System.out.print(i);
            else System.out.print(sb);
            System.out.println();
        }
    }
}


Creating an object out of an array of functions Here's a function that allows you to convert an array of functions into an object of functions where each property is set to the name of the function.

var _ = require("underscore")

function functionName(fn){
  //http://stackoverflow.com/a/17923727/340688
  if(fn.name) return fn.name
  return /^function\s+([\w\$]+)\s*\(/.exec(fn.toString())[1]
}

function objecfify(arr){
  return _.chain(arr)
    .map(function(fn){
      return [functionName(fn), fn]
    })
    .object()
    .value()
}

Usage:

var model = objecfify([
  function create(){
    return "create"
  },
  function read(){
    return "read"
  },
  function update(){
    return "update"
  },
  function remove(){
    return "delete"
  }
])

console.log(model)

Thoughts?

Why use the underscore at all.

For simple array manipulation, the objecfify can be modified as below:

function objecfify(arr) {
    var res = {};
    for (var i = 0, l = arr.length; i < l; i++) {
        res[functionName(arr[i])] = arr[i];
    }
    return res;
}


Interface of a variant-like class I have created an NBT format reader (NBT is a binary serialization format that Minecraft uses to store its data). I made a node-like class that can be a data entry or the root of a subtree (that's how NBT is defined). The public interface of it currently looks like this:

class Tag
{
    Tag();

    void read(std::istream& stream);
    void write(std::ostream& stream);

    void print(int indent = 0);

    int8_t byteValue() const;
    int16_t shortValue() const;
    int32_t intValue() const;
    int64_t longValue() const;
    float floatValue() const;
    double doubleValue() const;
    ByteArray byteArrayValue() const;
    IntArray intArrayValue() const;
    UTF8String stringValue() const;
    std::vector<Tag> children() const;

    UTF8String name() const;
};

Is this the right design though? Originally I wanted to do the *Value methods with templates, but the problem is that the value of a Tag is dynamic, so that does not play nicely with the compile-time genericity.

Is it a blasphemy not to make the various types derived classes? I couldn't really take advantage of a virtual value method because the return types would need to differ. Is there a better way to implement a data structure like this? The aim is to make it easy to use and fast.

Couple of things I noticed:

The serialize members:

void read(std::istream& stream);
void write(std::ostream& stream);

I don't particular mind them but I also would prefer to have operator<< and operator>> defined (Note you can simply make these call write/read respectively).

With the stream iterators defined this becomes less useful.

void print(int indent = 0);

The following seem like very simialer.

int8_t byteValue() const;
int16_t shortValue() const;
int32_t intValue() const;
int64_t longValue() const;
float floatValue() const;
double doubleValue() const;
ByteArray byteArrayValue() const;
IntArray intArrayValue() const;
UTF8String stringValue() const;

I think we can replace all of them with:

template<typename T>
T const& get() const;

I like this better as we do not need to guess that byte maps to int8_t or short maps to int16_t as we can explicitly get the type we want:

 int8_t  val = t.get<int8_t>();

But there is already something very similar defined in boost:

boost::any


Cleaning a WPA wordlist I have a short bash script that processes gigs and gigs of data.  I am looking for any improvements to make it faster.  This is my very first bash script so please be gentle. I am really only concerned about the while loop. The rest of it is fine I think. It's the while loop where the real work is done and could use the most enhancement.

#!/bin/bash

# This script will clean a WPA wordlist
# It will read every line of the given file
#       Remove all whitespace except for newlines
#       Delete the line if it is less than 8 chars or greater than 63
# It will then exit with time of execution

IFS=$'\n'       # make newlines the only separator

startTime=$(date)       # start time of execution
fileToClean=$1          # this is the file we will be sanatizing, 1st cmd line arg
deletedlines=0          # number of lines that did not meet WPA PSK critera
validlines=0            # number of lines that were valid PSKs  and added to file

if [ -z $fileToClean ]; then #No file specified
        echo ""
        echo "No file specified!"
        echo ""
        exit -1
fi

if [ ! -f $fileToClean ]; then #File does not exist
        echo ""
        echo "File not found!"
        echo ""
        exit  -1
fi

#By this point I am assuming I entered a valid file and will begin cleaning
echo ""
echo 'Cleaning word list: ' $1
echo "Start Time: " $startTime
echo ""

while read line; do  #read every line in file and save to var line

        line="${line##*( )}"        # trim leading whitespace
        line="${line%%*( )}"        # trim trailing whitespace

        if [ ${#line} -ge 8 ] && [ ${#line} -le 63 ]; then     # if trimmedline length >= 8 && <= 63
                echo  $line >> $outputfile
                ((validlines++))
                continue
        fi

        ((deletedlines++))

done <$fileToClean

utime="$( TIMEFORMAT='%lU';time ( ls ) 2>&1 1>/dev/null )"  # this stores the executio time in the var utime

echo "Processing completed, it took" $utime "and $deletedlines were deleted."
echo $validlines "were added to the output file "$outputfile" as they were valid PSKs"
echo ""

The if statement can be reduced to just:

[[ ${#line} >= 8 && ${#line} <= 63 && $((++validLines)) > 0 ]]
    && echo $line >> $outputfile || ((deletedlines++))

We use bash's [[ ... ]] shell keyword to collate our conditions together. Helpfully, [[ ... ]] supports the use of >= and <=.

[[ <something-to-test> ]] && <one-command-if-test-is-true> || <one-command-if-test-is-false> is quite a useful (IMHO!) construct in bash for implementing a if-then-else logic. The only thing to note is that you can only specify one command each for both branches.

As such, we can 'inline' our increment of validLines and do an always true comparison to simplify our if branching, namely to either (&&) output the line to the output file, or (||) increment our deletedLines. Otherwise, it gets slightly longer due to the required use of { ... } compound command:

[[ ${#line} >= 8 && ${#line} <= 63 ]]
    && { ((validLines++)); echo $line >> $outputfile; } || ((deletedlines++))

Now onto the other parts...

fileToClean should be set as fileToClean="$1" in case the filename has spaces inside.
Multi-line echo can be done as such:

[ ! -f "$fileToClean" ] && cat<<M && exit -1

Oh noes,
We have a multi-line error message
Telling user that the file is not a regular file.

M

Your way of checking the elapsed time should be done outside your script, using the time command. time can generate more (and probably more accurate) statistics about the 'performance' of your script and is readily available, hence the suggestion as opposed to manually calculating the value(s) yourself. :)

Now do you really need to do this in bash? Here's an awk one-liner (split into three for readability):

awk '{sub(/^[ ]+/,"");sub(/[ ]+$/,"")};
    length()>=8&&length()<=63{v++;print > "'"$outputfile"'" };
    END{print "Valid lines:",v"\nInvalid lines:",NR-v}' "$fileToClean"

Trim line.
If length matches, count the line and print to "$outputfile".
At the end, print the summary for valid/invalid lines.


update list based on other values Basically I have a 2d list containing movies. Movies are specified by name, version and description. For example

['Jaws', 1, 'Movie about sharks']  - where Jaws = name, 1 = version and Movie about sharks = description.

My result is a dictionary which contains a map between name and description. But name should be updated to contain the version for e.g

['Jaws', 2, 'Movie about more sharks'] - should now be :

{'JawsV2': 'Movie about more sharks'}

Is there a more pythonic way to do this?

def movie_version_map():
    movies =  [['Jaws', 1, 'Movie about sharks'],
               ['Jaws', 2, 'Movie about more sharks'],
               ['HarryPotter', 1, 'Movie about magic'],
               ['HarryPotter', 4, 'Movie about more magic']]
    for movie in movies:
        if movie[1] != 1:
            movie[0] = ''.join([movie[0], 'V',str(movie[1])])

    newversion = dict([(movie[0],movie[2]) for movie in movies])
    return newversion

Output

{'Jaws': 'Movie about sharks', 'HarryPotter': 'Movie about magic', 'HarryPotterV4': 'Movie about more magic', 'JawsV2': 'Movie about more sharks'}

A simple dict comprehension and use of str.format() will do the job here:

>>> {"{}V{}".format(name, version): description
     for name, version, description in movies}
{'HarryPotterV1': 'Movie about magic',
 'HarryPotterV4': 'Movie about more magic',
 'JawsV1': 'Movie about sharks',
 'JawsV2': 'Movie about more sharks'}

Or, in very old versions of Python where dict comprehensions don't exist, simple replace with dict() and a generator expression - e.g: dict(... for ... in moves).

Note that if this is just because you want to have the data as keys, you don't need to turn them into a string, a tuple can be a key too:

>>> {(name, version): description
     for name, version, description in movies}
{('Jaws', 1): 'Movie about sharks',
 ('HarryPotter', 4): 'Movie about more magic',
 ('Jaws', 2): 'Movie about more sharks',
 ('HarryPotter', 1): 'Movie about magic'}

This would be more appropriate where you don't need the strings, as it means you don't have to parse stuff out or create strings for keys, and makes the data easier to manipulate.


SQL query to select all teachers not in different table For homework I had to do the following:

  Give a list of all the teachers that don't have a classroom assigned to them.

It involves the following 2 tables:

Code inside Groups table would be the classroom in this case and TeacherId matches the Id inside Teachers table.

My solution to the problem is the following query:

SELECT Id, FirstName, MiddleName, LastName
FROM Teachers
WHERE Id NOT IN (SELECT TeacherId FROM Groups)

It works perfectly, however, I wonder if there was a better solution using JOINs.

Edit:

I should mention there is a CONSTRAINT on the Groups table:

ALTER TABLE Groups
ADD CONSTRAINT [FK_Groups_Teachers]
FOREIGN KEY (TeacherId)
REFERENCES [Teachers] ([Id])

Your query using NOT IN is good, but you can also use LEFT JOIN and keep only the teachers who don't have Groups:

SELECT t.Id, FirstName, MiddleName, LastName
FROM Teachers t LEFT JOIN Groups g ON t.Id=g.TeacherId
WHERE g.TeacherId IS NULL


Map square coordinates in matrix to flat array indices without hurting performance I'm more like playing and the problem I'm going to demonstrate is a learning task, so to speak, not something I really need to solve.

Imagine having a matrix of data. You chose square in it and would like to access each point on square perimeter by 1D index, like this:

This image represents instance of some Matrix class, such as:

Matrix m = ...;
std::cout<<m.data[0][1]; // "foo", first row, second column

Now I would like to create wrapping class Subsquare which, if represents the yellow square on the image works like this:

// Params: x, y, size, parent matrix
Subsquare sqr(1, 1, 6, m); // create from matrix

std::cout<<sqr[4]; //"4", as per image

I did try to make the class, but I got lost when trying to invent the algorithm to perform the mapping. I'd also like the algorithm to be optimal.

This is my attempt:

/**
 * @brief The Subsquare class allows you to iterate over square section of matrix
 */
class Subsquare {
public:
    /**
     * @brief Subsquare
     * @param x coordinate, 0-indexed
     * @param y coordinate, 0-indexed
     * @param length size of the square, it will span length items with it's side
     * @param parent matrix that contains the data
     */
    Subsquare(const int x, const int y, const int length, const Matrix& parent)
        : x(x)
        , y(y)
        , length(length)
        // the trick is that we only can count square corners once
        , items(length*4-4)
        , m(parent)
    {}
    /**
     * @brief The Side enum is helper enum for determining from which side are we picking index
     *
     */
    enum Side:byte {
        TOP,
        RIGHT,
        BOTTOM,
        LEFT
    };
    /**
     * @brief operator [] provides access to nth item in square, starting
     *                    at top left corner and going clockwise
     * @param index the index, 0-indexed
     * @return
     */
    int operator[] (int index) const {
        const byte side = index/(length-1);
        const int position = index%(length-1);
        //std::cerr<<"Pos: "<<index<<" side: "<<(int)side<<" subpos: "<<position<<'\n';
        switch (side) {
        case TOP:
            return m.data[y][x+position];
            break;
        case RIGHT:
            return m.data[y+position][x+length-1];
            break;
        case BOTTOM:
            return m.data[y+length-1][x+length-1-position];
            break;
        case LEFT:
            return m.data[y+length-1-position][x];
            break;
        }
    }
    /**
     * @brief Allows iterating over the square using for(int val: square) {}
     */
    class iterator {
    public:
        iterator(const int pos, const Subsquare& sq) :
            pos(pos), parent(sq)
        {}
        int pos;
        const Subsquare& parent;
        int operator*() {
            return parent[pos];
        }
        bool operator!=(const iterator& other) {
            return other.pos!=pos;
        }
        void operator++() {
            ++pos;
        }
    };
    /// Iterator helper methods
    iterator begin() const {
        return iterator(0, *this);
    }
    iterator end() const {
        return iterator(items, *this);
    }

    const Matrix& m;
    const int x;
    const int y;
    const int length;
    const int items;
};

Now this works correctly, it seems:

1 2 3
4 5 6
7 8 9
1, 2, 3, 6, 9, 8, 7, 4,

However I'm quite concerned about performance. Is there a way to make the algorithm faster while keeping it comfortable to use? The task I'm working on involves searching squares in very big matrix and the algorithm must be fast.

I see nothing wrong with the concept.
Code looks reasonable to me.

Iterator

You implementation of iterator does not meet the requirements for a standard iterator so you may have issues using it with some of the standard algorithms.

See: http://en.cppreference.com/w/cpp/iterator/iterator

You are missing some standard types in the iterator:

Member type Definition
iterator_category   Category
value_type  T
difference_type Distance
pointer Pointer
reference   Reference

Currently your iterator can only be considered an Input Iterator but given its simplicity it really should be Random Access Iterator.

http://en.cppreference.com/w/cpp/iterator/iterator_tags

Which means it needs to support the following:

http://en.cppreference.com/w/cpp/concept/RandomAccessIterator

Accesses

Currently you only have const accesses:

int operator[] (int index) const {

Normally you would expect to see non const access as well

int& operator[] (int index) {

And maybe a version that validates its parameters.

int  at(int index) const {
int& at(int index) {

Also since you currently don't allow write access through operator[] your iterator is actually const_iterator. There are several version of iterator that you can implement.

iterator       begin();
const_iterator begin() const;
const_iterator cbegin() const;

Might also be worth adding the reverse versions of the iterator but that might also be overkill.


Python generate custom combinations of sub lists I'm generating a custom combination of sub lists without using itertools.

Here's what I've come up with:

input =  [[1, 2], [2, 3], [4, 3]]
output = [[1, 2, 2, 3], [1, 2, 4, 3], [2, 3, 4, 3]]

def getList():
    a = [[1, 2], [2, 3], [4, 3]]
    return(a)

c=[]                        # output list
l = len(getList())
for i in range(l):
    for j in range(i+1,l):
        a=getList()
        a[i].extend(a[j])
        c.append(a[i])

As the extend() updates the input list, I defined a function to redefine the input. Is it a recommended practice? What could be improved here?

What could be improved here?

@thnx Peilonrayz, a generator expression will work better on large inputs

Use a nested generator expression.
DON'T hardcode variables, this will make your code very static.
Make a function, to accept different variables.

def custom_combo(L):
    return (L[i] + L[j] for i in range(len(L)) for j in range(i+1, len(L)))

if __name__ == '__main__':
    a = [[1, 2], [2, 3], [4, 3]]
    print(list(custom_combo(a)))


Node.js backend login logic So I wrote a rather primitive login logic in Node.js, which authenticates the user and handles JWT. Is it any good in terms of security, efficiency, building, async/sync, logging. SECURITY is my main concern.
On in a prettier format the question would be:

SECURITY: Is my website secure in any way, shape, or form? I'm wondering if I could implement any security measures other than the ones that are built in to the methods provided by Node.js. Also, I know the passwords are plainly obvious to guess, but they are like that to ensure logging in as different users works.
EFFICIENCY: Is how I'm checking usernames and password efficient? Is there any better way to do this?
BUILDING: Is how I loaded my website acceptable? Reading from a file and then ending the response?
ASYNC/SYNC: I know I preform async and sync calls at the same time. Is there any problem to this?
LOGGING: I log all connections to the server, and all login attempts. Is this a good practice, or am I overdoing what logging is supposed to accomplish?
(Source: Login Server with Node.js)

My code is:
// main login logic
app.post('/api/login', apiLimiter, async function (req, res) {
  // TODO: implement cookie check whether a still valid token is set and if so respond with cookie already set
  // TODO: add roles into jwt and add roles checking into other code
  // TODO: if wrong password send a response telling there's a wrong password/username
  try  {
    const pw = req.body.password;
    const submittedUser = req.body.email;

    User.findOne({eMail: req.body.email}, async function (err, user) {
      if (err) throw err;
      console.log(user);
      const match = await bcrypt.compare(pw, user.password);
      console.log(match);
      if (match === true && user.eMail == submittedUser) {
        jwt2.sign({user}, 'secrettokenhere', { expiresIn: '15min'}, (err, token) =>{
          res.cookie(
            'access_token', 'Bearer '+ token, {
              //domain: 'localhost',
              path: '/',
              expires: new Date(Date.now() + 900000), // cookie will be removed after 15 mins
              httpOnly: true // in production also add secure: true
            })
            .json(
              user
            );
        });
      }
      else {
        res.status(200).send("Bad login");
      }
    });
  } catch (err) {
    res.status(500).send();
    console.log(err);
  }
});

P.S. there's gonna be a follow-up separate question with Frontend logic.

Async login bug The try/catch around User.findOne does not accomplish anything because findOne is asynchronous. When findOne fails, it'll pass an error to the callback, but when the callback does throw err, nothing is there to catch the asynchronous error, so no response will be sent to the user. Another issue is that you aren't checking if user exists - if it doesn't, an error will be thrown when you try to access its password property. (You also aren't checking for whether .sign results in an error or not)
You could also consider using Promises instead of callbacks - findOne already returns a Promise, and Promises are often preferable because they're more easily chainable and their error handling can be cleaner.
Security looks reasonable, though:

Make sure connections are only permitted over HTTPS. HTTP requests are interceptable.
Even if the login bug above gets fixed, your current implementation is capable of informing anyone whether a given email is registered. For example, if I know your email address, I can try to login as you and examine the response to see whether the password was wrong or if no such email exists. This may not be desirable. If you wanted to fix it, have when the password doesn't match enter the same block as when the email isn't found, to make it indistinguishable to the user - you'd want to send a reply of Bad login in both cases.
On a similar note, when authentication fails, the appropriate status code to send is 401 (unauthorized), not 200.

Is how I'm checking usernames and password efficient?

Looks completely normal to me.

LOGGING: I log all connections to the server, and all login attempts. Is this a good practice

If one is logging, login attempts are one of the most important things to log. But console.log is not the right way to do it, at least not alone - say that some user was concerned about their logins, how would you examine their recent login attempts? Control-F-ing through the application stdout isn't a very manageable way of doing it. I'm not sure what the industry standard for this is, but you could consider saving to a logging database.
Email login You do:
const pw = req.body.password;
const submittedUser = req.body.email;

User.findOne({eMail: req.body.email}, async function (err, user) {
  if (err) throw err;
  console.log(user);
  const match = await bcrypt.compare(pw, user.password);
  console.log(match);
  if (match === true && user.eMail == submittedUser) {

You put the request email into a variable named submittedUser, which doesn't sound all that intuitive to me - better to use a variable name that indicates that it contains an email string, not a user, like email - you could destructure both at once with:
const { password, email } = req.body;

Then, later, use those variables instead of going through req.body again.
After the .findOne, there shouldn't be any need to do user.eMail == submittedUser - that check should be superflouous given the findOne call's constraint.
The body contains email, but the database contains eMail. The capitalization is different, which is an odd inconsistency which could cause typos and bugs. I'd recommend using a single property name - probably email, since that capitalization is much more common.
Rather than comparing the match against === true, you can just check if the match is truthy - or, another option, to reduce unnecessary indentation would be to throw if the match isn't truthy, and handle errors in a .catch.
Since .sign is callback-based, but you want to work with Promises, make it Promise-based with util.promisify.
Overall I'd hope to make the code look something like this:
const jwtSignPromisified = util.promisify(jwt2.sign).bind(jwt2);
app.post('/api/login', apiLimiter, async function (req, res) {
    const failLogin = () => {
        logLoginAttempt(email, false); // or something - 2nd param indicates success
        // could also pass IP address
        res.status(401).send('Bad login');
    };
    const { password, email } = req.body;
    try {
        const user = await User.findOne({ email });
        if (!user) return failLogin();
        const match = await bcrypt.compare(password, user.password);
        if (!match) return failLogin();
        logLoginAttempt(email, true);
        const token = await jwtSignPromisified({ user }, 'secrettokenhere', { expiresIn: '15min' });
        res.cookie(
            'access_token', 'Bearer ' + token, {
            //domain: 'localhost',
            path: '/',
            expires: new Date(Date.now() + 900000), // cookie will be removed after 15 mins
            httpOnly: true // in production also add secure: true
        })
            .json(user);
    } catch (error) {
        // This should not be entered under normal circumstances:
        logServerError(error);
        res.status(500).send('Unexpected server error');
    }
});

where logLoginAttempt and logServerError save to logging databases that can be examined.


Kotlin: Comparing enums I am currently converting a project from Java to kotlin and re-writing the code in the process to make the most out of kotlin's capabilities. The project contains a Battle System which uses elements to establish inferiority/superiority or neutrality of attacks based on the attacker's and defender's element. The code is much more concise than the java version but I have the feeling there is an even better way to do it, I am just not sure what that way is, so any input would be greatly appreciated, also in regard to improvements in regard to the current code:

internal class Element {

    enum class Type {
        NEUTRAL {
            override fun superiorTo(): List<Type> = emptyList()
            override fun inferiorTo(): List<Type> = emptyList()
        },
        FIRE {
            override fun superiorTo(): List<Type> = listOf(EARTH)
            override fun inferiorTo(): List<Type> = listOf(WATER)
        },
        WATER {
            override fun superiorTo(): List<Type> = listOf(FIRE)
            override fun inferiorTo(): List<Type> = listOf(WIND)
        },
        WIND {
            override fun superiorTo(): List<Type> = listOf(WATER)
            override fun inferiorTo(): List<Type> = listOf(EARTH)
        },
        EARTH {
            override fun superiorTo(): List<Type> = listOf(WIND)
            override fun inferiorTo(): List<Type> = listOf(FIRE)
        };

        private val typeCount = Type.values().size

        abstract fun superiorTo(): List<Type>
        abstract fun inferiorTo(): List<Type>

        fun compare(defender: Type): ComparisonResult {
            val result: ComparisonResult
            if (inferiorTo().contains(defender)) {
                result = ComparisonResult.INFERIOR
            } else if (superiorTo().contains(defender)) {
                result = ComparisonResult.SUPERIOR
            } else {
                result = ComparisonResult.NEUTRAL
            }
            return result
        }
    }

    enum class ComparisonResult {
        NEUTRAL, SUPERIOR, INFERIOR
    }
}

The reason I am going for Lists instead of single types is twofold:

account for the option to have inferiorities/superiorities towards
multiple elements
but mostly to make it easy to take the Neutral element into account and treat it just like all the others

You could use a bidirectional map. In Java syntax this would be:

BidiMap<Type, Type> superior = new BidiMap<>();
superior.put(EARTH, WIND);
superior.put(WIND, WATER);
superior.put(WATER, FIRE);
superior.put(FIRE, EARTH);

That's it. This greatly reduced the redundancy of your code, as well as the verbosity. Since it is a BidiMap, you can query it in both directions.

An alternative approach is to define an array ComparisonResult[typeCount][typeCount] cmp and initialize that using similar calls as above, again avoiding redundancy. Then your compare method can just return cmp[this.ordinal ()][other.ordinal()].


Leetcode - product of array except self Given an array nums of n integers where n > 1,  return an array output
  such that output[i] is equal to the product of all the elements of
  nums except nums[i].

Example:

Input:  [1,2,3,4]
Output: [24,12,8,6]
Note: Please solve it without division and in O(n).

  Follow up: Could you solve it with constant space complexity? (The
  output array does not count as extra space for the purpose of space
  complexity analysis.)

My solution

public class ProductExceptSelf {
    /**
     * Approach 1:- Find the left[i] at i containing all the left multiplication. Do same with right[i]
     */
    public int[] productExceptSelf(int[] nums) {

        int size = nums.length;

        int[] output = new int[size];
        int[] left = new int[size];
        int[] right = new int[size];

        int temp = 1;
        // Start filling up left array
        for (int i = 0; i < size; i++) {
            left[i] = temp;
            temp = temp * nums[i];
        }

        // Again set the temp to 1
        temp = 1;

        // Start filling up right array
        for (int i = size - 1; i >= 0; i--) {
            right[i] = temp;
            temp = temp * nums[i];
        }

        // Final traversal and return sum array.
        for (int i = 0; i < size; i++) {
            output[i] = left[i] * right[i];
        }

        return output;
    }
}

Please help me to improve Auxilary space, code readibility and any best practice if I am missing.

Every time you feel obliged to put a comment like

    // Start filling up left array

you really deal with a potentially important algorithm, which deserves a name, and a method of its own. In this case the algorithm is known as partial product. Consider factoring it out, and see the comments disappear.

Ditto for the second loop.

Regarding space complexity,

You don't need left: accumulate the partial products directly into output.
You don't need right as well: instead of accumulating the partial products, use it immediately:

    temp = 1;

    for (int i = size - 1; i >= 0; i--) {
        output[i] *= temp;
        temp = temp * nums[i];
    }


Print out all possible letter combinations a given phone number can represent I am working on a problem in which given a 7 digit telephone number, we need to print out all possible combinations of letters that each number could represent.

I came up with below code and I was wondering if there is any way to optimize it or if there is any better way? As of now complexity is O(n3).

public class LetterCombinationsOfPhoneNumber {
  private static final HashMap<Character, String> map = new HashMap<>(8);
  static {
    map.put('2', "abc");
    map.put('3', "def");
    map.put('4', "ghi");
    map.put('5', "jkl");
    map.put('6', "mno");
    map.put('7', "pqrs");
    map.put('8', "tuv");
    map.put('9', "wxyz");
  };

  public static List<String> letterCombinations(String digits) {
    LinkedList<String> results = new LinkedList<>();
    results.add("");

    for (int i = 0; i < digits.length(); i++) {
      String letters = map.get(digits.charAt(i));
      for (int j = results.size(); j > 0; j--) {
        String intermediateResult = results.poll();
        for (int k = 0; k < letters.length(); k++) {
          results.add(intermediateResult + letters.charAt(k));
        }
      }
    }
    return results;
  }

  public static void main(String[] args) {
    System.out.println(letterCombinations("23"));
  }
}

This can certainly be optimized.
Your algorithm works in loops that iterate over all permutations, placing one letter each time.
Let us take a different approach:

The number of output combinations can be calculated in advanced: multiply the number of letters for all input numbers. in yout example ("23") the calculation is
number 2 has 3 letters
number 3 has 3 letters
output combinations will be 3*3=9

Furthermore, we can calculate which letter will be placed in each combination based on the combination index. if we assume combination index goes from 0 to 8:
for number 2 (first number in input)
- combinations 0-2 will contain 1st letter
- combinations 3-5 will contain 2nd letter
- combinations 6-8 will contain 3rd letter
for number 3 (2nd number in input)
- combinations 0,3,6 will contain 1st letter
- combinations 1,4,7 will contain 2nd letter
- combinations 2,5,8 will contain 3rd letter

so the formula for letter placement is based on the combination index, the position of the number in the input and the position of the letter in the letters-of-number String.

the complexity of this algorithm is o(n*m) where n is number of input letters and m number of output combinations.

one comment regarding the code: You use the results as a Queue.  the LinkedList is an implementation of this interface. For clarity sake, the variable should be defined with the type that states its usage. This is also true for the map variable (should be defined as Map)


Decode FEN string into separate variables for the 6 categories FEN strings are compact representations of chess board positions which allows you to derive the necessary information to start playing a chess game from that position. This includes things like the pieces on the board, castle statuses, etc. More information here: https://en.wikipedia.org/wiki/Forsyth%E2%80%93Edwards_Notation
I have written a simple function in go to decode a FEN string into separate variables with suitable types for each of the desired pieces of information that FEN strings represent, and I would like to see if I am parsing the FEN string in a reasonable manner.
import (
    "strings"
)

type pair struct {
    x, y int8
}

func decodeFen(fen string) ([8][8]int8, int8, [4]bool, pair, int8, int) {
    split := strings.Split(fen, " ")
    var board [8][8]int8
    board_split := strings.Split(split[0], "/")
    for r, row := range board_split {
        var col_ind int8
        for _, char := range row {
            if char >= 49 && char <= 57 {
                for i := int8(0); i < int8(char) - 48; i++ {
                    board[r][col_ind] = '-'
                    col_ind++
                }
            } else {
                board[r][col_ind] = int8(char)
                col_ind++
            }
        }
    }
    var color int8
    if split[1] == "w" {
        color = 1
    } else {color = 2}
    var castle_status [4]bool
    var status_map = map[rune]int8 {
        'K' : 0, 'Q' : 1, 'k' : 2, 'q' : 3}
    if split[2] != "-" {
        for _, char := range split[2] {
            castle_status[status_map[char]] = true
        }
    }
    var ep_pos pair = pair{-1, -1}
    if split[3] != "-" {
        x_label := split[3][0]
        y_label := split[3][1]
        y_label -= 48
        ep_pos = pair{int8(x_label - 97), int8(y_label - ((y_label - 1) - (8 - y_label)) - 1)}
    }
    var halfmove_clock int8 = int8(split[4][0]) - 48
    var fullmove_number int = int(split[5][0]) - 48
    return board, color, castle_status, ep_pos, halfmove_clock, fullmove_number
}

This is really just a parsing problem, and to split the strings I decided to use the Split() function from the strings go library, rather than writing my own. Any improvements and feedback for this code would be great, and I apologize if I am not correctly asking this question as this is my first post in this community.

Code should be correct, maintainable, robust, reasonably efficient, and, most importantly, readable.

To be certain that the code is correct, carefully read the specification and related documents.
Forsyth–Edwards Notation
PGN-SPEC
Portable Game Notation
Algebraic notation (chess)
Chess
For a code review, compare the specification and the code implementing it side-by-side.

In chess, there are pieces placed on squares which are organized into ranks and files to form a board. There doesn't appear to be any references to these basic concepts, apart from board, in decodeFen. Therefore, comparison of code to specification is difficult.
"PGN is "Portable Game Notation", a standard designed for the representation of chess game data using ASCII text files. PGN data is represented using a subset of the eight bit ISO 8859/1 (Latin 1) character set." Therefore,
var board [8][8]int8

is incorrect. Characters are not type int8 (8-bit signed integers). Characters are type byte (8 bits).
var board [8][8]byte

The code
for _, char := range row { ... }

expects a UTF-8 encoded string. UTF-8 encoding is not the same as ISO 8859/1 encoding.
Go is a safe, statically typed language. The code
board[r][col_ind] = int8(char)

is a failed attempt to correct errors when ISO 8859/1 encoding is decoded as UTF-8 encoding.
The decodeFen code is a "stream-of-conciousness" code that is hard to read and prove correct. The code is missing fundamental concepts like functions to encapsulate complexity and implementation details. For example, organize decodeFen as a series of calls to parse functions for each FEN field.

As an example, consider the FEN piece placement field. Here's a first draft:
pieces.go:
package main

import (
    "errors"
    "fmt"
    "strings"
)

type Board [8][8]byte

const (
    whitePieceLetters = "PNBRQK"
    blackPieceLetters = "pnbrqk"
    pieceLetters      = whitePieceLetters + blackPieceLetters
)

func isPiece(p byte) bool {
    for i := 0; i < len(pieceLetters); i++ {
        if p == pieceLetters[i] {
            return true
        }
    }
    return false
}

var (
    ErrRankOutOfRange = errors.New("rank out of range")
    ErrFileOutOfRange = errors.New("file out of range")
    ErrPieceInvalid   = errors.New("piece invalid")
)

func parseFENPieces(board *Board, pieces string) error {
    ranks := strings.Split(pieces, "/")
    if len(ranks) != len(board) {
        return ErrRankOutOfRange
    }
    for r, rank := range ranks {
        f := 0
        for i := 0; i < len(rank); i++ {
            piece := rank[i]
            if piece >= '0' && piece <= '9' {
                for j := byte(0); j < piece-'0'; j++ {
                    if f >= len(board[0]) {
                        return ErrFileOutOfRange
                    }
                    board[r][f] = '-'
                    f++
                }
            } else {
                if f >= len(board[0]) {
                    return ErrFileOutOfRange
                }
                if !isPiece(piece) {
                    return ErrPieceInvalid
                }
                board[r][f] = piece
                f++
            }
        }
        if f != len(board[0]) {
            return ErrFileOutOfRange
        }
    }
    return nil
}

func main() {
    fens := []string{
        // https://en.wikipedia.org/wiki/Forsyth%E2%80%93Edwards_Notation
        "rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1",
        "rnbqkbnr/pppppppp/8/8/4P3/8/PPPP1PPP/RNBQKBNR b KQkq e3 0 1",
        "rnbqkbnr/pp1ppppp/8/2p5/4P3/8/PPPP1PPP/RNBQKBNR w KQkq c6 0 2",
        "rnbqkbnr/pp1ppppp/8/2p5/4P3/5N2/PPPP1PPP/RNBQKB1R b KQkq - 1 2",
    }
    var board Board
    for _, fen := range fens {
        fields := strings.Split(fen, " ")
        if len(fields) > 0 {
            pieces := strings.Split(fen, " ")[0]
            fmt.Println(pieces)
            err := parseFENPieces(&board, pieces)
            fmt.Println(err)
            for i := range board {
                fmt.Printf("%c\n", board[i])
            }
        }
    }
}

Playground: https://play.golang.org/p/-pPnCs2GMao
Output:
rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR
<nil>
[r n b q k b n r]
[p p p p p p p p]
[- - - - - - - -]
[- - - - - - - -]
[- - - - - - - -]
[- - - - - - - -]
[P P P P P P P P]
[R N B Q K B N R]

Use chess terminology to match the specification.
The chess board data structure is ubiquitous. Give it a type:
type Board [8][8]byte

In Go, arguments are passed by value. For an array, the value is the entire array. Therefore, for efficiency, we use a pointer (8 bytes or 4 bytes) rather than an array (64 bytes).
Input must be valid. Make no assumptions about external input. Report errors.
Don't use magic values: 49 is '0', 57 is '9'.
And so forth.


Limiting the save content of file to certain size The goal here is to only save (to file) the most recent Objects created if Object::maxMemoryOfObjects is exceeded.

#include <iostream>
#include <list>
#include <algorithm>
#include <fstream>
#include <map>

struct Object {
    const int ID;
    Object() : ID(++lastObjectID) {}
    static const int maxMemoryOfObjects = 200;
    static int totalMemoryOfObjects, lastObjectID;
};
int Object::totalMemoryOfObjects = 0, Object::lastObjectID = 0;

struct Location {
    std::list<Object*> objectsLyingAround;
    Location() {
        for (int i = 0; i < 12; i++) {
            Object* object = new Object;
            objectsLyingAround.push_back(object);
            Object::totalMemoryOfObjects += sizeof(*object);
        }
    }
};

struct Person {
    void visitLocation (const Location*) {}
};

std::list<Location*> itinerary;

void saveObjects() {
    std::ofstream outfile ("Objects.txt");
    std::map<int, Object*/*, std::greater<int>*/> objectMap;
    for (Location* x : itinerary)
        for (Object* o : x->objectsLyingAround)
            objectMap.emplace (o->ID, o);
    std::cout << "Objects being saved (from oldest to newest) IDEALLY are:\n";
    for (const auto& x : objectMap)
        std::cout << x.first << ' ';
    if (Object::totalMemoryOfObjects > Object::maxMemoryOfObjects) {
        std::cout << "\nAlert!  totalMemoryOfObjects > maxMemoryOfObjects!  Will save only the newest objects.\n";
        auto it = objectMap.begin();
        while (Object::totalMemoryOfObjects > Object::maxMemoryOfObjects) {
            Object::totalMemoryOfObjects -= sizeof(*it->second);
            std::cout << "totalMemoryOfObjects reduced to " << Object::totalMemoryOfObjects << std::endl;
            it = objectMap.erase(it);
        }
        std::cout << "Objects being saved (from oldest to newest) NOW are:\n";
        for (const auto& x : objectMap)
            std::cout << x.first << ' ';
    }
outfile << objectMap.size() << std::endl;
for (const auto& x : objectMap)
    outfile << x.first << ' ';
}

void loadObjects() {
    int numObjects;
    std::ifstream infile ("Objects.txt");
    infile >> numObjects;
    // etc...
}

int main() {
    Person sam;
    for (int i = 0; i < 5; i++) {
        Location* nextLocation = new Location;
        itinerary.push_back (nextLocation);
        sam.visitLocation (nextLocation);
    }
    saveObjects();
    loadObjects();
}

Output:

Objects being saved (from oldest to newest) IDEALLY are:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30
 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 5
7 58 59 60
Alert!  totalMemoryOfObjects > maxMemoryOfObjects!  Will save only the newest objects.
totalMemoryOfObjects reduced to 236
totalMemoryOfObjects reduced to 232
totalMemoryOfObjects reduced to 228
totalMemoryOfObjects reduced to 224
totalMemoryOfObjects reduced to 220
totalMemoryOfObjects reduced to 216
totalMemoryOfObjects reduced to 212
totalMemoryOfObjects reduced to 208
totalMemoryOfObjects reduced to 204
totalMemoryOfObjects reduced to 200
Objects being saved (from oldest to newest) NOW are:
11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37
 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60

But the way I'm doing it ironically adds more memory to the Object class (its ID data member). Is there a better way to do this?  We assume here that the Objects are being saved and loaded many times until megabytes or even gigabytes of data are being stored in memory, at which a max should be enforced.  Really old items that are lying around in Locations but never picked up will assume to be discarded first since they are probably forgotten anyway.

First: Crappy indentation makes it hard to read.

Your algorithm seems unnecessarily complicated. Why not work out how many items you want to serialize and just serialize those items.

 // Work out how many items we will need to drop.
 std::size_t  countOfItems      = objectMap.size();
 std::size_t  maxNumberOfItems  = Object::maxMemoryOfObjects/sizeof(Object);

 std::size_t  advance  = (countOfItems > maxNumberOfItems)
                             ? (countOfItems - maxNumberOfItems)
                             : 0;

 // Get an iterator to the first item
 // Then advance it forward the number of items we want t drop
 // These are the older items (as map is a sorted container).
 auto printer = objectMap.begin();
 std::advance(printer, advance);

 // Print all the items using std::copy and a stream iterator
 // May not work out the box for you but you should learn how to do this.
 std::copy(printer, objectMap.end(), std::ostream_iterator<Object>(outfile));

Memory Ownership

Your code is littered with pointers.
This is not a good idea in general (but hard to tell without more context in this case).

RAW Pointers are very rare in C++ as they do not convey object ownership. Usually pointers are managed by some owning object that manages their lifespan (as a result you never interact with them directly but via their owning object).

You should be using containers of object (not pointers) or if you have object hierarchies then use "pointer containers" that understand their content is a pointer and can act accordingly. If you don't have pointer containers available (they are in boost) then you should at least be using smart pointers to maintain the concept of ownership.

Looks like an error:

for (const auto& x : objectMap)
    outfile << x.first << ' ';
}

You are not serializing the object but only the ID. I think you meant:

for (const auto& x : objectMap)
    outfile << x.second << ' ';
}

But now you also need to define:

std::ostream operator<<(std::ostream& str, Object const& data)
{
   returns str << /* Serialize object */
}
std::istream operator>>(std::istream& str, Object& data)
{
   returns str >> /* De-Serialize object */
}

Looks like it might be an issue.

Object::totalMemoryOfObjects += sizeof(*object);

The size of an object in memory is always fixed. So you may as well count the number of objects. Note: sizeof() is computed at compile time not runtime. If you want to add up the number of bytes in things like strings you need to write your own method to do it.

But when you use operator<< the object is generally serialized and the number of bytes can vary (unless you write your object in binary fixed size format to the stream (but then you need to worry about machine peculiarities like endianness (so unless there is an extreme need for speed it is usually best to use a non binary format)).

Declare variables JUST before you use them

std::ofstream outfile ("Objects.txt");

// 500 lines of code not using outfile

outfile << objectMap.size() << std::endl;
for (const auto& x : objectMap)
    outfile << x.first << ' ';
}

Move the declaration of outfile to the point just before you tart using it. In C++ because of constructors/destructors the side affects may allocate resources. If you don't need them don't use them so wait until you need them.

It also has the benefit of making the declaration near the code that uses it so it makes it real easy to see the type of the object you are using (as it is just there with the code).

Encapsulation

struct Person {

You seem to be making all your objects publicly accessible. This does not help encapsulation. You should make all your data members private by default. Then provide methods (which are verbs) that act on the object.

class Object
{
    static int const maxMemoryOfObjects   = 200;
    static int       totalMemoryOfObjects = 0;
    static int       lastObjectID         = 0;

    static int allocateNextAvailableID()
    {
        return ++lastObjectID;
    }
    const int ID;
    public:
        Object()
            : ID(allocateNextAvailableID())
        {}

        // define an ordering on your objects.
        friend bool operator<(Object const& lhs, Object const& rhs)
        {
             // With an ordering you can use Object as the key.
             // in a sorted container.
             return lhs.ID < rhs.ID;
        }
};

int main()
{
    std::set<Object>     objectSet; // No need to allow access to the ID
                                    // to external entities that do not need to know.
                                    // provide the appropriate access method (in this
                                    // case `operator<()` that defines the ordering.
}


Digit sum loops+random positive number generator I have to prepare program that summing the digits of two given by user numbers and then give back the random number which is

natural number
the digit sum of this number is bigger then the digit sum of given by user numbers

It's look like everything is ok but i dont know if it is just luck in random number generator or it's working well.
Thank you for review.

import java.util.Random;

import java.util.Scanner;

public class Ex1 {

public static void main(String[] args) {
    Random rnd = new Random();
    Scanner scn = new Scanner(System.in);
    int sum1 = 0;
    int sum2 = 0;
    int sum3 = 0;
    int sum4 = 0;
    System.out.println("1TH NUMBER : ");
    int a1 = scn.nextInt();
    System.out.println("2ND NUMBER : ");
    int a2 = scn.nextInt();
    System.out.println((0 > a1 || 0 > a2 ? "ERROR-NEGATIVE NUMBER" : "OK"));
    while (a1 > 0) {
        sum1 += a1 % 10;
        a1 /= 10;
    }
    //System.out.println(sum1);
    while (a2 > 0) {
        sum2 += a2 % 10;
        a2 /= 10;
    }
    //System.out.println(sum2);
    int temp = sum1 + sum2; //temporary-for storage /=
    while (temp > 0) {
        sum3 += (temp) % 10;
        (temp) /= 10;
    }
    // System.out.println(sum3);
    while (true) {
        int a3 = rnd.nextInt(Integer.MAX_VALUE);
        sum4 += (a3) % 10;
        (a3) /= 10;
       // System.out.println(sum4);
        if (sum4 > sum3) {
            System.out.println(a3 + " this is my number");
            break;
        }
    }

}
}

I have some suggestion for you.

Code duplication

In your code, you have some code duplication that can be extracted in a method. By extracting the code, the code will become shorter, be less error-prone and easier to read.

I suggest that you make a method to print a question and read the user input.

public static void main(String[] args) {
   //[...]
   int a1 = askQuestionAndReceiveAnswer(scn, "1TH NUMBER : ");
   int a2 = askQuestionAndReceiveAnswer(scn, "2ND NUMBER : ");
   //[...]
}

private static int askQuestionAndReceiveAnswer(Scanner scn, String s) {
   System.out.println(s);
   return scn.nextInt();
}

Since the logic is the same to handle the sum, you can extract both of the while into a method. This extraction will remove lots of code!

public static void main(String[] args) {
   //[...]
   int sum1 = getSum(a1);
   int sum2 = getSum(a2);

   int temp = sum1 + sum2; //temporary-for storage /=

   int sum3 = getSum(temp);
   //[...]
}

private static int getSum(int userInput) {
   int currentSum = 0;
   while (userInput > 0) {
      currentSum += userInput % 10;
      userInput /= 10;
   }
   return currentSum;
}

Other observations

In my opinion, I would extract the last calculation in a method and return the result.

public static void main(String[] args) {
   //[...]
   int number = findNumber(rnd, sum3);
   System.out.println(number + " this is my number");
   //[...]
}

private static int findNumber(Random rnd, int sum3) {
   int sum4 = 0;
   while (true) {
      int a3 = rnd.nextInt(Integer.MAX_VALUE);
      sum4 += (a3) % 10;
      (a3) /= 10;
      if (sum4 > sum3) {
         return a3;
      }
   }
}

Refactored code

public static void main(String[] args) {
   Random rnd = new Random();
   Scanner scn = new Scanner(System.in);

   int a1 = askQuestionAndReceiveAnswer(scn, "1TH NUMBER : ");
   int a2 = askQuestionAndReceiveAnswer(scn, "2ND NUMBER : ");
   System.out.println((0 > a1 || 0 > a2 ? "ERROR-NEGATIVE NUMBER" : "OK"));

   int sum1 = getSum(a1);
   int sum2 = getSum(a2);
   int temp = sum1 + sum2; //temporary-for storage /=

   int sum3 = getSum(temp);

   int number = findNumber(rnd, sum3);
   System.out.println(number + " this is my number");
}

private static int findNumber(Random rnd, int sum3) {
   int sum4 = 0;
   while (true) {
      int a3 = rnd.nextInt(Integer.MAX_VALUE);
      sum4 += (a3) % 10;
      (a3) /= 10;
      if (sum4 > sum3) {
         return a3;
      }
   }
}

private static int getSum(int userInput) {
   int currentSum = 0;
   while (userInput > 0) {
      currentSum += userInput % 10;
      userInput /= 10;
   }
   return currentSum;
}

private static int askQuestionAndReceiveAnswer(Scanner scn, String s) {
   System.out.println(s);
   return scn.nextInt();
}


Reverse string in JavaScript without using reverse() I have to reverse a string in JavaScript and cannot use the built in reverse() function. It does not seem efficient to have to create two arrays but is there a better way?

function reverseString(str) {
  newarr = str.split("");
  result = [];
  x = newarr.length;
  for (i = x; i > -1; i--) {
    result.push(newarr[i]);
  }
  str = result.join("");
  return str;
}

reverseString("hello");

One array to rule them all
You do not need to create two arrays; one is enough. Simply swap slots until you've reached the "middle" of the array.

Declare your variables
It is better to use the var keyword to define your variables. Otherwise, they are declared on the top-level scope, i.e; they become global, and may be accessed and modified by other functions.

Why not use reverse()?
You do not say why you cannot use built-in functions. If you happen to have some base code which overrides native functions, then some groaning and roaring is in order.

function reverse(str) {
  var chars = str.split("");
  var length = chars.length;
  var half = length / 2;
  for (var ii = 0; ii < half; ii++) {
    var temp = chars[ii];
    var mirror = length - ii - 1;
    chars[ii] = chars[mirror];
    chars[mirror] = temp;
  }
  return chars.join("");
}

console.log(reverse("abcd"));
console.log(reverse("abcde"));


Checking if a value is in the RGB range and then increment/decrement Is there a better way to check if the variable x is in the 0-255 range and:

if x is less than 255, add colVel to it
if x equals 255 minus colVel from it.

My implementation, in Python 3.8.
if 0 <= x < colVel:
    flag = True
if (255-colVel) < x < 255:
    x = 255
if (255-colVel) <= x <= 255:
    flag = False
if flag == True:
    x += colVel
if flag == False:
    x -= colVel

This section of code is a function that creates iridescence while an object moves. This is all of my code, and I'm happy for it to be reviewed.
import pygame
import random
pygame.init()

win = pygame.display.set_mode((800,600))

pygame.display.set_caption('Chachi')
x = 50
y = 50
width = 2
height = 2
vel = 5
colVel = 10
rf = True
gf = True
bf = True
running = True

def irid(r, g, b, rFlag=True, gFlag=True, bFlag=True):
        def rgbRange(x, flag):
                if 0<= x< colVel:
                        x = random.randint(1, 100)
                        flag = True
                if (255-colVel) < x < 255:
                        x = 255
                if (255-colVel) <= x <= 255:
                        flag = False
                if flag==True:
                        x += colVel
                if flag==False:
                        x -= colVel
                return x, flag
        r, rFlag = rgbRange(r, rFlag)
        g, gFlag = rgbRange(g, gFlag)
        b, bFlag = rgbRange(b, bFlag)
        return r, g, b, rFlag, gFlag, bFlag
r = irid(0, 255, 0)[0]
g = irid(0, 255, 0)[1]
b = irid(0, 255, 0)[2]

while running:
    pygame.time.delay(1)

    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            running = False
    keys = pygame.key.get_pressed()

    if keys[pygame.K_LEFT]:
        x -= vel
        r, g, b, rf, gf, bf = irid(r, g, b, rf, gf, bf)

    if keys[pygame.K_RIGHT]:
        x += vel
        r, g, b, rf, gf, bf = irid(r, g, b, rf, gf, bf)
    if keys[pygame.K_UP]:
        y -= vel
        r, g, b, rf, gf, bf = irid(r, g, b, rf, gf, bf)
    if keys[pygame.K_DOWN]:
        y += vel
        r, g, b, rf, gf, bf = irid(r, g, b, rf, gf, bf)

    pygame.draw.rect(win, (r, g, b), (x, y, width, height) )
    pygame.display.update()

pygame.quit()

Avoid repeating yourself
First, avoid having separate variables for r, g and b. It means you have to write all code three times. Just store the three components in a list, like so:
color = [0, 255, 0]

Then you can pass color to other functions, and have those functions iterate over the elements of the list.
Better way to create a triangle wave
What you are doing is basically creating a triangle wave shape for the intensity of each color component. Instead of keeping the current color value and a flag to store whether you are on the increasing slope or decreasing slope, just store a variable that holds the phase in a full cycle of the triangle wave.
In this case, I would store an integer between 0 and 511 for each color component. Initialize the phase like you did the color vector before:
phase = [0, 255, 0]

When updating the phase, do:
for i in range(3):
    phase[i] += colorVel
    phase[i] %= 512

Of course, this gives a sawtooth wave, and the values you get will be too high, but now you can map it onto a triangle wave with the correct amplitude:
color = list(map(lambda x: x if x < 256 else 511 - x, phase))

Create a class to store the state of the iridescence
Even better would be to have a class manage all the internal state of an iridescence color, and allow it to be called as a function that generates the next color:
class Iridecence:
    def __init__(self, r, g, b, colVel=10):
        self.phase = [r, g, b]
        self.colVel = colVel

    def __call__(self):
        for i in range(3):
            if self.phase[i] < self.colVel:
                self.phase[i] = random.randint(1, 100)
            self.phase[i] += self.colVel
            self.phase[i] %= 512;
        return list(map(lambda x: x if x < 256 else 511 - x, self.phase))

Now you can use this as follow in your code:
irid = Iridescence(0, 255, 0)
color = irid()

while running:
    ...
    if keys[pygame.K_LEFT]:
        x -= vel
        color = irid()
    ...
    pygame.draw.rect(win, color, (x, y, width, height) )
    pygame.display.update()


Shopping cart in Rails for karaoke items This is my first cart implementation in Rails so I wanted to get some feedback on the correct method to implement it.

First of all, I am storing the data directly in the session because the items in the cart are small.

The idea is that I save a list of integers(ids of the items) in session[]. When a user clicks on "Add to Cart" then the id of that product will be added to the list stored in the rails session variable.

Application controller

class ApplicationController < ActionController::Base
  # Prevent CSRF attacks by raising an exception.
  # For APIs, you may want to use :null_session instead.
  protect_from_forgery with: :exception

  before_action :set_cart

  private

    def set_cart

      if session[:cart]
        @cart = session[:cart]
      else
        session[:cart] = []
        @cart = session[:cart]
      end

    end

end

Main index file lists entries with "Add to Cart" button for each entry. After the user clicks on "Add to Cart" then the add_item method of CartController is called.

  <% @karaokes.each do |karaoke| %>

      <%= link_to karaoke.name, karaoke_path(karaoke) %>
      <%= link_to "Add to Cart", add_path(karaoke) %>

  <% end %>

Cart Controller looks as follows:

class CartController < ApplicationController

    def add_item

       k = Karaoke.find(params[:id])

       if !@cart.include?(k.id)
        @cart.push(k.id)
       end

       redirect_to cart_path

    end

    def list_items

       @items = @cart

    end

    def remove_item

       @cart.delete_at(params[:index].to_i)

       redirect_to cart_path

    end

end

Finally the cart page looks as follows which lists all the items added to cart.

 Cart

<% @items.each_with_index do |itm, index| %>

        <%= Karaoke.find(itm).name %>
        <%= link_to "Remove", remove_path(index) %>

<% end %>

Generally looks great.

Since @cart is basically a list of Karaoke ids, you could update the list_items method to look like that:

def list_items
  @items = Karaoke.find(@cart)
end

Then the cart view can be transformed into:

<h1> Cart </h1>
<% @items.each do |item| %>
  <p>
    <%= item.name %>
    <%= link_to "Remove", remove_path(item) %>
  </p>
<% end %>

Using a Set instead of plain array enables us to remove the presence checking and make sure items are always unique in the cart:

class ApplicationController < ActionController::Base
  ...

  def set_cart
    session[:cart] ||= []
    @cart = Set.new(session[:cart])
  end
end

class CartController < ApplicationController
  def add_item
   @cart.add(karaoke_id) if Karaoke.exists?(karaoke_id)

   redirect_to cart_path
  end

  def remove_item
    @cart.delete(karaoke_id)

    redirect_to cart_path
  end

  def karaoke_id
    params[:id].to_i
  end
end

As a side note, I know it's kind of a personal preference, but consider using 2 spaces for indentation as per the style guide.


Running alarms in python My code is designed to simply trigger alarms; it is not supposed to be used by anyone but me.

import subprocess,time,threading, winsound, sys, basepipe
commands = ''
def alarm_checker(alarms):
    """This will be the main threaded process, and will be the meat of the program."""
    global commands
    hours = [int(a.split(":")[0]) for a in alarms]
    mins = [int(a.split(":")[1]) for a in alarms]
    while True:
        localtime = time.localtime()
        for hour,minute in zip(hours,mins):
            if localtime.tm_hour == hour and localtime.tm_min == minute:
                guiserver,guiclient = basepipe.create_double()
                process = subprocess.Popen(r"c:\python34\pythonw.exe c:\python34\guibutton.py",stdout=guiclient)
                while guiserver.read() == '':
                    winsound.Beep(1000,2000)
                hours.remove(hour)
                mins.remove(minute)
        if commands != '':
            for y in commands.split("
"):
                try:
                    exec(y)
                except BaseException as e:
                    sys.stderr.write(str(e))
            commands = ''
        time.sleep(2)
if __name__ == "__main__":
    today = time.localtime().tm_wday
    everyday = ['6:55','7:15','8:12']
    advising = ['13:25','13:55']
    tuethur = ['10:55']
    monday = everyday + advising + ['15:00']
    tuesday = everyday + tuethur
    wednesday = everyday + ['3:50']
    thursday = everyday + advising + tuethur
    friday = everyday
    week = [monday,tuesday,wednesday,thursday,friday,['25:00'],['25:00']]
    thread = threading.Thread(target=alarm_checker,args=(week[today],))
    thread.start()

The basepipe program is a wrapper around windows named pipes in Python. All it does is attempt to implement them in the same way as any other type of IO. One of the peculiarities of windows named pipes is that client read operations are always blocking, while server read operations always raise an error when there is no data in the buffer. I changed this error to simply return ''. create_double() creates a pair of server/client objects. The guibutton.py program does nothing other than open a tcl/tk window. Winsound provides direct access to the sound card, and 1000,2000 means 1000 hertz for 2 seconds, which is annoying as hell.

Instead of splitting each item in alarms to hours and minutes and then zipping them again like this:

hours = [int(a.split(":")[0]) for a in alarms]
mins = [int(a.split(":")[1]) for a in alarms]

for hour,minute in zip(hours,mins):
    # ...

You could do it in one go without zipping:

hours_minutes = [[int(x) for x in a.split(":")] for a in alarms]
for hour, minute in hours_minutes:
    # ...

It's recommended to follow the coding style guide PEP8.

For example you could simplify things like this:

    if commands != '':

To this:

    if commands:

It's also recommended to avoid multiple imports on the same line like this:

import subprocess,time,threading, winsound, sys, basepipe

And it would be better to move all code out of the if __name__ == "__main__": guard into a def main(): method to avoid variable shadowing in the other methods called from there, as the code in the if guard is in global scope.


Jquery sortable style function This is a basic sorting function written in jQuery that moves items in the DOM around to create empty spaces for a droppable(). Since there is so much going on in a droppable over event, i'd love some feedback on how this could be done better.

This is all in a function called on the over event of the droppable()

I have an array of empty spots

    var emptyHolders = [];

    $('.pipeline-holder').each(function(){
        if($(this).hasClass('holder-empty')){
            var eid = $(this).attr('id');
            emptyHolders.push(eid.substring(14));
        }
    });

cid is the droppable element that you're over top of, so I find the next open slot using

   for (var i = 0; i < emptyHolders.length; i++) {
        var currentEmpty = emptyHolders[i];
        if (currentEmpty > cid) {
            nextEmpty = currentEmpty;
            i = emptyHolders.length;
        } else {
            prevEmpty = parseInt(currentEmpty);
        }
    }

If there is a an empty slot further down the list, I use a for loop to loop through moving the items around the DOM to make the space needed to drop the element.

 if (nextEmpty != null) {
        var moveMe = nextEmpty -1;

        for (var i = moveMe; i >= cid; i--) {

            var nextcount = i + 1;
            var me = $('#pipeline-rank-' + i);
            var next = $('#pipeline-rank-' + i).parents('.pipeline-rank-row').next().find('.pipeline-holder');
            var pid = $('#pipeline-rank-' + i).find('.content-wrapper').attr('id');

            next.append($('#pipeline-rank-' + i).find('.content-wrapper'));
            next.removeClass('holder-empty');
            next.siblings('.remember-my-position-hover').html(pid);
        }
         $('#pipeline-rank-' + cid).addClass('holder-empty');
    }

If there is not one further down the list, I do the same thing in reverse to check if there is a spot above the droppable to push items up into.

It's usable here: http://jsfiddle.net/mstefanko/LEjf8/9/

Any thoughts are extremely appreciated!

Here you have some general thoughts:

First of all, cache jQuery elements (var $this = $(this); instead of multiple executions of $(this)). http://jsfiddle.net/tomalec/pXPdS/1/
You can also chain jQuery methods, it simplifies codes, and helps with above.
To reduce amount of code you can use toggleClass instead of addClass removeClass.http://jsfiddle.net/tomalec/pXPdS/2/
Instead of selecting $('#rankDrag') in start callback you can use reference given in arguments ui.helper
http://jsfiddle.net/tomalec/pXPdS/3/
To get id of empty/not empty holder ids, in my opinion, .map is more readable than .each. Moreover, you can move that function outside rearrange to avoid re-declaring it every time.

function getTruncatedId(index, element){
    return element.id.substring(14);
}

function rearrange(dropid) {
    //..
    emptyHolders = holders.filter('.holder-empty').map(getTruncatedId);
    isRanked = holders.filter(':not(.holder-empty)').map(getTruncatedId);
    //..
}

http://jsfiddle.net/tomalec/pXPdS/4/
Pass dropped element to rearrange, so you will not have to traverse through DOM again:

rearrange( $(this).attr('id') );//dropId = $(this).attr('id');
//..
isEmpty = $('#'+dropId).hasClass('holder-empty');

http://jsfiddle.net/tomalec/pXPdS/5/


Efficiency of looping though a text map to draw objects I am making a game that uses a text map to draw obstacles and courses for the player to navigate. However, I am concerned that my method of looping through the text map to draw objects on the screen isn't as efficient as it could be.

View the playable project on Khan Academy.

Here is my code dealing with the text map:

var platforms = [];
platforms.add = function(xPosition, yPosition, width, height, colour) {
    platforms.push( new Block(xPosition, yPosition, width, height, colour) );
};

platforms.display = function() {
    for (var i = 0; i < platforms.length; i++) {
        platforms[i].display();
    }
};
var levelOne = [
    // each symbol is equivalent to 40 pixels
    ["##########"], // # - block
    ["#        #"], // S - starting position
    ["#        #"],
    ["#        #"],
    ["#        #"],
    ["#  #     #"],
    ["#       ##"],
    ["#     #  #"],
    ["#S  #    #"],
    ["##########"],
];
var symbol;
var playerX;
var playerY;
for(var i = 0; i < levelOne.length; i++) {

    for(var j = 0; j < levelOne[i].length; j++) {
        for(var v = 0; v < levelOne[i][j].length; v++) {
            var symbol = levelOne[i][j][v];
            if(symbol === "#") {
            // Platform arguments: X, Y, Width, Height, Color
                platforms.add(v*width/10, i*height/10, width/10, height/10, color(0, 174, 255));
            } else if (symbol === "S") {
                playerX = v*width/10;
                playerY = i*height/10;
            }
        }
    }
}

How can I make this code more efficient?

Well, for starters, you could reduce operations to \$O(n^2)\$ by skipping your third loop:

var levels = [
    ["##########",
     "#        #",
     "#        #",
     "#        #",
     "#        #",
     "#  #     #",
     "#       ##",
     "#     #  #",
     "#S  #    #",
     "##########"]
];

for(var i = 0; i < levels[0].length; i++){
    for(var j = 0; j < levels[0][i].length; j++){
        var symbol = levels[0][i][j];
        console.log(symbol);
    }
}

I changed levelOne to a levels array where you can refer to each level by index, since you probably want to add more levels later on.

jsFiddle

Why do you define symbol outside of the loop, and re-declare it inside?

Regarding your platforms:

var platforms = [];

platforms.add = function(xPosition, yPosition, width, height, colour) {
    platforms.push(new Block(xPosition, yPosition, width, height, colour) );
};

You are just declaring an add method for that specific array instance. It also creates an enumerable property on the array which isn't good if someone happens to use for..in to iterate the array (which I wouldn't recommend in the first place). Why not simply call push directly?

If you insist on creating your own add method, in my opinion, platforms should be either its own "type" (if you plan to create several instances):

function platform(){
    this.blocks = [];
}

platform.prototype.addBlock(function(block){
    this.blocks.push(block);
});

Or if you know that you'll only use one platform, create a "static" object:

var platform = {
    blocks: [],
    addBlock: function(block){
        this.blocks.push(block);
    }
};