# Reduce number of operations to get one array value

I can't wrap my head around that. There seem to be a lot of redundancy, and even if it is not called called more than ten times in the main loop, I still find it hard to swallow that it loops through the whole list at each invocation.

def get_file_info(f, stream, field):

discoverer = GstPbutils.Discoverer()
discoverer.connect('discovered', on_discovered)
info = discoverer.discover_uri(f)

for vinfo in info.get_video_streams():
ftype = (vinfo.get_stream_type_nick())
width = vinfo.get_width()
height = (format(vinfo.get_height()))
depth = (format(vinfo.get_depth()))
framerate = ('Frate: {}'.format(vinfo.get_framerate_num()))
bitrate = ('Brate: {}'.format(vinfo.get_max_bitrate()))
par = vinfo.get_par_num()
fname = vinfo.get_caps().to_string().split(',')[0]

if stream == 'video':
if field == 'duration':
return str(convert_time(info.get_duration(), 'hh:mm:ss'))
elif field == 'width':
return str(width)
elif field == 'height':
return str(height)
elif field == 'type':
return str(ftype)
elif field == 'depth':
return str(depth)
elif field == 'framerate':
return str(framerate)
elif field == 'bitrate':
return str(bitrate)
elif field == 'par':
return str(par)
elif field == 'name':
return str(par)


I read the lambda documentation, but can't seem to write a proper one, but I would sure like to.

• Are the fields either "duration", or "width" or .... "name", or can it contain any string? – Stewie Griffin Jun 29 '16 at 7:35
• Yes! they are just a fixed set of no more than the nine listed here ; No, it can not contain any string. – yPhil Jun 29 '16 at 7:41
• As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. – BCdotWEB Jun 29 '16 at 8:30

@Stewie Griffin's answer is good, but I dare say it promotes eval, where dict is safer and more expansive.
The main aim here is to use the same if's as Stewie, as they are definitely good. As a dictionary contains things and you can't get other things out of it it's safer. With Stiewies for example you can pass 'get_file_info' as the field and it'll give you '<function get_file_info at 0x03335150>'. It also means that you can return a dictionary for each and every stream in info.get_video_streams().

To obtain this we will make a variable video_info that will hold the needed information.

video_info = {
'width': 2,
'height': 3,
'name': 'Name of video'
}

field = 'width'
str(video_info[field])
Out[3]: '2'

field = 'name'
str(video_info[field])
Out[5]: 'Name of video'


This means we just need to move all the information into this dictionary. And so you'll need to change the for-loop to create a dictionary.

video_info = {
'type': vinfo.get_stream_type_nick(),
'width': vinfo.get_width(),
'height': format(vinfo.get_height()),
'depth': format(vinfo.get_depth()),
'framerate': 'Frate: {}'.format(vinfo.get_framerate_num()),
'bitrate': 'Brate: {}'.format(vinfo.get_max_bitrate()),
'par': vinfo.get_par_num(),
'fname': vinfo.get_caps().to_string().split(',')[0]
}


I'd also go on to say that you can change the code to return a dictionary and you can then remove all the field stuff from this function. And you can use a list comprehension to return a dictionary for every info.get_video_streams().

def get_file_info(f, stream):
discoverer = GstPbutils.Discoverer()
discoverer.connect('discovered', on_discovered)
info = discoverer.discover_uri(f)
duration = convert_time(info.get_duration(), 'hh:mm:ss')
return [
{
'duration': duration,
'type': vinfo.get_stream_type_nick(),
'width': vinfo.get_width(),
'height': format(vinfo.get_height()),
'depth': format(vinfo.get_depth()),
'framerate': 'Frate: {}'.format(vinfo.get_framerate_num()),
'bitrate': 'Brate: {}'.format(vinfo.get_max_bitrate()),
'par': vinfo.get_par_num(),
'fname': vinfo.get_caps().to_string().split(',')[0],
}
for vinfo in info.get_video_streams()
]

def get_file_field(f, stream, field):
videos = get_file_info(f, stream)
video_info = videos[-1]
if stream == 'video':
return str(video_info[field])

• I'm confused by the get_file_info return code. You are returning a list that contains a dict (I think commas missing between key:value pairs), and then a for loop? I've never seen anything like this – joel goldstick Jun 29 '16 at 9:52
• @joelgoldstick Thank you, I forgot to add commas in my dictionary, ): I'm using a list comprehension to return a list. It's like map but simpler to write. I can't really find anything that explains it too well, but these may help, and it's a way to remove the l=[];for a in ...: l.append(a); return l idiom. – Peilonrayz Jun 29 '16 at 10:24
• This is beautiful :) – yPhil Jun 29 '16 at 19:10

As far as I can tell, the first part of you code looks fine.

However, all the elif calls can be avoided. The first if is necessary, since you're calling convert_time. The others can be simplified, if you know the fields are one of the 9 you are listing. Instead of checking if the names are similar, and then call str(string), you can use the field name directly, in combination with locals().

I believe this is a better way to do it:

if stream == 'video':
if field == 'duration':
return str(convert_time(info.get_duration(), 'hh:mm:ss'))
else
return str(locals()[field])


locals()[field] will return whatever field contains. Then, str(locals()[field]) will convert it to a string.

width = 2
height = 3
name = 'Name of video'

field = 'width'
str(locals()[field])
Out[5]: '2'

field = 'name'
str(locals()[field])
Out[7]: 'Name of video'


Note, I'm not the most experienced Python user, so it's possible that there are better ways to do this.

• A safer alternate to this is to use dict. eval is ok, but may lead to bad or unsafe habits. – Peilonrayz Jun 29 '16 at 8:25
• Thanks @JoeWallis =) I didn't know it could be used here. I'm quite sure both me, OP and others would be happy to see a review that explains how to solve this in a better, safer way =) I'll look into how dict can be used here when I have the time =) – Stewie Griffin Jun 29 '16 at 8:32
• @StewieGriffin just replace eval(field) with locals()[field] – jacg Jun 29 '16 at 8:56
• @StewieGriffin or use a pre-meditated explicit dict for this purpose in the for-loop: d = {'ftype': (vinfo.get_stream_type_nick()), 'width': vinfo.get_width(), ...} and then you can use d[field] instead of eval(field) – jacg Jun 29 '16 at 9:01
• But using locals you could query fields not intended for that use, like discoverer`. Using a dictionary, like Joe suggested is much better idea. – CodesInChaos Jun 29 '16 at 15:01